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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Feb 07, 2002 at 03:27:14PM +0000, Gregory Berkolaiko wrote:
> I just got some free time to write a report on this patch.
> But, as I see it's committed already.
> Oh, well, my report is generally favourable anyway, it's just a pity that
> I missed another chance to fix the rating mistake.  See below.

If you were some hours faster...

>  --- Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote: 
> > 
> > Attached is an cleanup for the cleanup patch. Gregory didn't made the
> > patch and I didn't have time till now. It contains the usual stuff:
> >  - remove variables which aren't needed
> >  - move variables into the deepest possible scope
> >  - add {}
> >  - feed through indent
> >  - add constants and macros to make the code more clear
> 
> My opinion is that one-use constants like
> UNKNOWN_FITNESS_PENALTY_PERCENT
> should not be made macros because:
> 1. It makes code longer : define, undefine, comment.  While you can just
> do with a comment.
> 2. It makes code more obscure: while you know what
> UNKNOWN_FITNESS_PENALTY_PERCENT
> means, to know it's value you have to scroll back and forth.  It's not
> worth the effort for something which is used only once.
> 3. The formatting of #define is not aestetically pleasing...

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

  /* reduce bla bla */
  a -= (a*6)/100;

is bad. IMHO each function should declare such constants at the start
of the function or file.

> Other macros are fine.
> However I would like to point out a similarity between
> 
> > +#define UNIT_RATING(punit, x, y, defence_multiplier) \
> > +  (UNIT_DEFENSE(punit, x, y, defence_multiplier) * (punit)->hp)
> 
> and 
> 
> int unit_vulnerability_basic(struct unit *punit, struct unit *pdef)
> {
>   int v;
> 
>   v = get_total_defense_power(punit, pdef) *
>       (punit->id ? pdef->hp : unit_type(pdef)->hp) *
>       unit_type(pdef)->firepower / 30;
> 
>   return(v);
> }
> 
> from aiunit.c
> 
> 30 is just a normalization.  But firepower is something which should be
> presnt in the defence/attack rating.  The general formula for the rating
> is    def/att_power * hp * firepower.
> 
> 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
make real changes in this area. Although I have made at least
two: 

-       if (punit->moves_left <= base_move_cost) {
+       if (punit->moves_left < base_move_cost) {

and

1/16 != 0.06

> > +    /* 
> > +     * Find the best ally unit at the target tile.
> > +     */
> > +    best_ally = NULL;
> > +    num_of_allied_units = 0;
> >      {
> > +      /* 
> > +       * Initialization only for the compiler since it is guarded by
> > +       * best_ally.
> > +       */
> > +      int rating_of_best_ally = 0;
> > +
> > +      unit_list_iterate(ptile->units, aunit) {
> > +   if (pplayers_allied(unit_owner(aunit), unit_owner(punit))) {
> > +     int rating_of_current_ally =
> > +         UNIT_RATING(aunit, x, y, defence_multiplier);
> > +     num_of_allied_units++;
> > +
> > +     if (!best_ally || rating_of_current_ally > rating_of_best_ally) {
> > +       best_ally = aunit;
> > +       rating_of_best_ally = rating_of_current_ally;
> > +     }
> > +   }
> > +      } unit_list_iterate_end;
> > +    }
> 
> [...]
> 
> > +      int rating_of_ally, rating_of_unit =
> > +     UNIT_RATING(punit, x, y, defence_multiplier);
> > +      
> > +      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.

> 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 ;)

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

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  Microsoft does have a year 2000 problem. I'm part of it. I'm running Linux.