[Freeciv-Dev] Re: AI cleanup Version 3 (PR#1170)

[Petr Baudis <pasky@xxxxxxxxxxx>]

Dear diary, on Sat, Dec 29, 2001 at 05:13:18PM CET, I got a letter,
where Raahul Kumar <raahul_da_man@xxxxxxxxx> told me, that...
> diff -ruN -Xdiff_ignore /home/rkumar/freeciv-clean/freeciv/ai/aiunit.c 
> freeciv/ai/aiunit.c
> --- /home/rkumar/freeciv-clean/freeciv/ai/aiunit.c    Sun Dec 30 00:50:42 2001
> +++ freeciv/ai/aiunit.c       Sun Dec 30 01:36:55 2001
> @@ -133,28 +133,29 @@
>  }
>  
>  /********************************************************************** 
> -  ...
> +  Amount of turns needed to reach destination x,y for the players unit.
> +  This will work for all units, including sea, air, land and igter units.
>  ***********************************************************************/
>  static int unit_move_turns(struct unit *punit, int x, int y)
...
Cleaning up this functions appears useless to me, as we have probably better
candidate for include, IIRC Greg made it, haven't we?
>   
>  /**************************************************************************
> @@ -1162,7 +1163,7 @@
>  **************************************************************************/
>  int find_something_to_kill(struct player *pplayer, struct unit *punit, int 
> *x, int *y)
>  {
> -  int a=0, b, c, d, e, m, n, v, i, f, a0, b0, ab, g;
> +  int a=0, b, min_turns_to_dest, d, e, move_rate, n, v, i, f, a0, b0, ab, g;
Ble - can you please put them at least to separate lines?
min_turns_to_dest seems too long for me - what about 'delay'? ;) (used that
in settlers code) - or maybe better 'move_time' :
move_time = move_cost / move_rate ;p. Doesn't it look lovely? ;))
>  #ifdef DEBUG
>    int aa = 0, bb = 0, cc = 0, dd = 0, bestb0 = 0;
>  #endif
> @@ -1225,8 +1226,8 @@
>    *x = punit->x; *y = punit->y;
>    ab = unit_belligerence_basic(punit);
>    if (!ab) return(0); /* don't want to deal with SIGFPE's -- Syela */
> -  m = unit_type(punit)->move_rate;
> -  if (unit_flag(punit, F_IGTER)) m *= SINGLE_MOVE;
> +  move_rate = unit_type(punit)->move_rate;
> +  if (unit_flag(punit, F_IGTER)) move_rate *= SINGLE_MOVE;
>    maxd = MIN(6, m) * THRESHOLD + 1;
>    f = unit_type(punit)->build_cost;
>    fprime = f * 2 * unit_type(punit)->attack_strength /
> @@ -1244,8 +1245,8 @@
>    if (ferryboat) really_generate_warmap(map_get_city(ferryboat->x, 
> ferryboat->y),
>                         ferryboat, SEA_MOVING);
>  
> -  if (ferryboat) boatspeed = (unit_flag(ferryboat, F_TRIREME) ? 6 : 12);
> -  else boatspeed = (get_invention(pplayer, game.rtech.nav) != TECH_KNOWN ? 6 
> : 12);
> +  if (ferryboat) boatspeed = (unit_flag(ferryboat, F_TRIREME) ? 2 * 
> SINGLE_MOVE : 4 * SINGLE_MOVE);
> +  else boatspeed = (get_invention(pplayer, game.rtech.nav) != TECH_KNOWN ? 2 
> * SINGLE_MOVE : 4 * SINGLE_MOVE);
I think putting the statements to separate lines would be better. And I would
probably rather do:

boatspeed = 2 * SINGLE_MOVE;

if ((ferryboat && unit_flag(ferryboat, F_TRIREME)) ||
    get_invention(pplayer, game.rtech.nav) != TECH_KNOWN)
   boatspeed *= 2;
>  
>    if (is_ground_unit(punit) && !punit->id &&
>        is_terrain_near_tile(punit->x, punit->y, T_OCEAN)) harborcity++;
> @@ -1270,17 +1271,17 @@
>  /* attempting to make empty cities less enticing. -- Syela */
>            if (is_ground_unit(punit)) {
>              if (!sanity) {
> -              c = (warmap.seacost[acity->x][acity->y]) / boatspeed; /* kluge 
> */
> -              if (boatspeed < 9 && c > 2) c = 999; /* tired of Kaput! -- 
> Syela */
> -              if (ferryboat) c += (warmap.cost[bx][by] + m - 1) / m + 1;
> -              else c += 1;
> -              if (unit_flag(punit, F_MARINES)) c -= 1;
> -            } else c = (warmap.cost[acity->x][acity->y] + m - 1) / m;
> +              min_turns_to_dest = (warmap.seacost[acity->x][acity->y]) / 
> boatspeed; /* kluge */
isn't the line too long? ;)
> +              if (boatspeed < 9 && c > 2) min_turns_to_dest = 999; /* tired 
> of Kaput! -- Syela */
please statement to separate line; obtw shouldn't it be move_time > 2 ?
> +              if (ferryboat) min_turns_to_dest += (warmap.cost[bx][by] + 
> move_rate - 1) / move_rate + 1;
ditto
> +              else min_turns_to_dest += 1;
> +              if (unit_flag(punit, F_MARINES)) min_turns_to_dest -= 1;
ditto
> +            } else min_turns_to_dest = (warmap.cost[acity->x][acity->y] + 
> move_rate - 1) / move_rate;
  +            } else {
  +              move_time = (warmap.cost[acity->x][acity->y] + move_rate - 1) 
/ move_rate;
  +            }
>            } else if (is_sailing_unit(punit))
  +          } else if (is_sailing_unit(punit)) {
> -             c = (warmap.seacost[acity->x][acity->y] + m - 1) / m;
> -          else c = (real_map_distance(punit->x, punit->y, acity->x, acity->y)
> -                 * SINGLE_MOVE) / m;
> -          if (c > 1) {
> +             min_turns_to_dest = (warmap.seacost[acity->x][acity->y] + 
> move_rate - 1) / move_rate;
too long line ;)
> +          else min_turns_to_dest = (real_map_distance(punit->x, punit->y, 
> acity->x, acity->y)
> +                 * SINGLE_MOVE) / move_rate;
  +          } else {
  +            move_time =
  +              (real_map_distance(punit->x, punit->y, acity->x, acity->y) *
  +               SINGLE_MOVE) / move_rate;
  +          }
  +
> +          if (min_turns_to_dest > 1) {
>              n = ai_choose_defender_versus(acity, punit->type);
>              v = get_virtual_defense_power(punit->type, n, acity->x, 
> acity->y) *
>                  unit_types[n].hp * unit_types[n].firepower *
> @@ -1291,7 +1292,7 @@
>                (!(acity->ai.invasion&1))) b -= 40; /* boats can't conquer 
> cities */
>            if (!punit->id
>             && !unit_really_ignores_citywalls(punit)
> -           && c > (player_knows_improvement_tech(aplayer, B_CITY) ? 2 : 4)
> +           && min_turns_to_dest > (player_knows_improvement_tech(aplayer, 
> B_CITY) ? 2 : 4)
ETOOLONG
>             && !city_got_citywalls(acity))
>           d *= 9;
>  
> @@ -1304,7 +1305,7 @@
>  
>            if (!is_ground_unit(punit) && !is_heli_unit(punit) && !pdef) b0 = 
> 0;
>  /* !d instead of !pdef was horrible bug that led to yoyo-ing AI warships -- 
> Syela */
> -          else if (c > THRESHOLD) b0 = 0;
> +          else if (min_turns_to_dest > THRESHOLD) b0 = 0;
What about explaining and elaborating b0 ? ;))
>            else if ((is_ground_unit(punit) || is_heli_unit(punit)) &&
>                     acity->ai.invasion == 2) b0 = f * SHIELD_WEIGHTING;
>            else {
...

Well, honestly, I would welcome rather patches for smaller scope, but much
higher focus, attempting to clean given function completely - this way it's
hard for me to review something, as only de-facto details are changed (sure, it
helps, but not much) and one can't see them in the context (as the context is
messy ;) and you can easily change them when you will clean this function by
yourself.

-- 

                                Petr "Pasky" Baudis

UN*X programmer, UN*X administrator, hobbies = IPv6, IRC, FreeCiv, (e)links
.
Firewall in a way is like the doorkeeper of a local pub. If you don't have your
I.D. on you, or if for some reason you do not qualify to enter, the doorkeeper
will not permit you to enter. In some extreme cases these doorkeepers will not
let you out, or at least give you a hard time before they finally let you out.
.
Public PGP key, geekcode and stuff: http://pasky.ji.cz/~pasky/