[Freeciv-Dev] Re: [PATCH] try to cleanup assess_danger (PR#1321)

[Raahul Kumar <raahul_da_man@xxxxxxxxx>]

--- Markus Linnala <maage@xxxxxxxxx> wrote:
> 
> Change variable names to what they mean.
> 

I have some problems with thse patches, and the sequel to this patch. I agree
with everything Petr already said on these patches, and believe the maintainers
should carefully contrast what Petr has done to this patch.



> diff -ur -X freeciv/diff_ignore freeciv/ai/advmilitary.c
> freeciv-cleanup-assess-danger-1/ai/advmilitary.c
> --- freeciv/ai/advmilitary.c  Wed Mar  6 12:05:09 2002
> +++ freeciv-cleanup-assess-danger-1/ai/advmilitary.c  Sun Mar 10 19:08:28 2002
> @@ -228,15 +228,14 @@
>  {
>    int i, danger = 0, v, dist, m;
>    Unit_Type_id utype;
> -  int danger2 = 0; /* linear for walls */
> -  int danger3 = 0; /* linear for coastal */
> -  int danger4 = 0; /* linear for SAM */
> -  int danger5 = 0; /* linear for SDI */
> +  int danger_wall = 0; /* linear for walls */
> +  int danger_coastal = 0; /* linear for coastal */
> +  int danger_sam = 0; /* linear for SAM */
> +  int danger_sdi = 0; /* linear for SDI */

I like the name change from danger[1,2,3,4]. This seems to already be in Petr's
CVS though.


> -  int urgency = 0;
> +  bool def_against_diplomat = FALSE; /* TRUE mean that this town can defend
> +                                   * against diplomats or spies */

I preferred the original diplomat, but this is really a matter of taste. If the
maintainers prefer to make it explicit, by all means.

>    bool igwall;

Should be bool igwall = FALSE;

>    int badmojo = 0;

No explanation of what this is.

>    int boatspeed;

Or this. 

[EVIL CODE]

>    unit_list_iterate(map_get_tile(pcity->x, pcity->y)->units, punit)
>      if (unit_flag(punit, F_PIKEMEN)) pikemen = TRUE;

> +    if (unit_flag(punit, F_DIPLOMAT)) def_against_diplomat = TRUE;
>    unit_list_iterate_end;
>  
>    players_iterate(aplayer) {
> @@ -286,7 +286,7 @@
>            igwall = unit_really_ignores_citywalls(funit);
>            if ((is_ground_unit(funit) && v != 0) ||
>                (is_ground_units_transport(funit))) {
> -            if (dist <= m * 3) urgency++;
> +            if (dist <= m * 3) pcity->ai.urgency++;
>              if (dist <= m) pcity->ai.grave_danger++;

This seems like a candidate for the combat calcs. 


>            v *= v;
>  
> -          if (!igwall) danger2 += v * m / dist;
> -          else if (is_sailing_unit(funit)) danger3 += v * m / dist;
> -          else if (is_air_unit(funit) && !unit_flag(funit, F_NUCLEAR))
> danger4 += v * m / dist;
> -          if (unit_flag(funit, F_MISSILE)) danger5 += v * m / MAX(m, dist);
> +          if (!igwall) danger_wall += v * m / dist;
> +          else if (is_sailing_unit(funit)) danger_coastal += v * m / dist;
> +          else if (is_air_unit(funit) && !unit_flag(funit, F_NUCLEAR))
> danger_sam += v * m / dist;
> +          if (unit_flag(funit, F_MISSILE)) danger_sdi += v * m / MAX(m,
> dist);
>            if (!unit_flag(funit, F_NUCLEAR)) { /* only SDI helps against
> NUCLEAR */
>              v = dangerfunct(v, m, dist);
>              danger += v;

[/EVIL CODE]


This entire block should be replaced by a calculation from the combat calcs.
I really do not like this. My reason for voting against this patch is based
mostly on this block of code. 


__________________________________________________
Do You Yahoo!?
Try FREE Yahoo! Mail - the world's greatest free email!
http://mail.yahoo.com/