[Freeciv-Dev] Re: [FreeCiv-Cvs] kauf: general cleanup of find_a_direction() as well ...

[Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx>]

 --- Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote: 
> On Thu, Feb 07, 2002 at 03:27:14PM +0000, Gregory Berkolaiko wrote:

<...>

> 
> What about a "const int"? But I think that

well, const is slightly better.
at least you know where to look for a variable initialisation.
but define can be put anywhere...

> 
>   /* reduce bla bla */
>   a -= (a*6)/100;
> 
> is bad. IMHO each function should declare such constants at the start
> of the function or file.

I really don't see why.
numerical constant is as transperent as you can get.
unless there are two places in which the constants should be the same...

if you want to change this factor, I guess it's marginally easier to find
it if it was given a name.  but if this factor is used only once there is
a great chance that you wouldn't know about it's existence until you
stuble upon it in the first place.

but I am not really bothered, I just think that making the code few lines
shorter is good provided you don't jeopardise readability and
flexibility.

> > So I guess it should be
> > #define UNIT_RATING(punit, x, y, defence_multiplier) \
> >  (UNIT_DEFENSE(punit, x, y, defence_multiplier) * (punit)->hp) \
> >  * unit_type(punit) -> firepower
> 
> I have just put the existing code in another form. I didn't want to

I know.
but I thought that maybe if I tell you this in time you'll pluck your
courage and do some structural change ;)

> > > +      assert((best_ally != NULL) == (num_of_allied_units > 0));
> > > +
> > > +      if (best_ally) {
> > > + rating_of_ally = UNIT_RATING(best_ally, x, y,
> defence_multiplier);
> > > +      } else {
> > > + rating_of_ally = 0;     /* equivalent of having 0 HPs */
> > > +      }
> > 
> > rating_of_ally = rating_of_best_ally from the above.
> > so don't really need the if-else and the "only for compiler" comment
> > above.
> 
> rating_of_best_ally is in another scope. And this is not a mistake. It
> is just a "private" variable of the find-best-ally part.

I know.  I just think that often you go to extremes to force some
concept.

> > could use aihandicap below.
> > 
> > > +      if (!map_get_known(tmp_x, tmp_y, pplayer)) {
> > > + if (punit->moves_left < base_move_cost) {
> > > +   /* Avoid the unknown */
> > > +   fitness[dir] -=
> > > +       (fitness[dir] * UNKNOWN_FITNESS_PENALTY_PERCENT) / 100;
> > 
> > 
> > why for(;;) is better than do-while?
> 
> Because so I could removed "dir" from the function scope ;)

hehe ;)


> 
> > > +  /*
> > > +   * Find random direction out of the best ones selected.
> > > +   */
> > > +  for (;;) {
> > > +    int dir = myrand(8);
> > > +
> > > +    if (fitness[dir] == best_fitness) {
> > > +      return dir;
> > > +    }
> > > +  }
> > >  


__________________________________________________
Do You Yahoo!?
Everything you'll ever need on one web page
from News and Sport to Email and Music Charts
http://uk.my.yahoo.com