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

[Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx>]

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.


 --- 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...

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

[...]

> +    /* 
> +     * 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.

[...]

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?

> +  /*
> +   * 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;


Anyway the patch is good,
G.

__________________________________________________
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