[Freeciv-Dev] Re: [PATCH] aiunit.c ai_manage_explorer cleanup (PR#1210)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sun, Jan 06, 2002 at 01:59:40PM -0800, Petr Baudis wrote:
> Hello,
> 
>   attached patch cleans the ai_manage_explorer function in aiunit.c of AI.
> Behaviour is not changed (autogames SHOULD remain same - I will do tests
> tommorow hopefully - now I've some weird problems with machine and I fear to
> run autogame now), it was only heavily commented and variable names were
> expanded to something meaningful. Oh, yes, and it seems to me that it 
> compiles.
> 
>   I think it's ready to actually go in, altough I'm open to objections and
> ideas how to extend/correct it even more.
> 
>   Note that this patch is obviously only third in very long line. It should be
> relatively light stuff and its path to CVS should be IMHO short. However, I'm
> not sure about some code (especially not why something is here, but why
> something is NOT here ;-), and I would like to receive some comments about it
> (the problem parts are marked in comments).
> 
>   After this will get in, I plan to jump away from this dull military stuff
> again and have a look to aicity.c probably. I just submit this because it's
> origins are in one of my oldest patch and it was waiting for include already
> pretty long time, so I decided to give it a try ;).
> 
> Happy hacking in new year,

>  /**************************************************************************
> -Explores unknown territory, finds huts.
> +Handle eXplore mode of a unit - explores unknown territory, finds huts.
           ^ This is a client-only gui thing

> +  /* The position of the unit; updated inside the function */
> +  int x, y;

Since also unit_id is initialized here x and y can also.

> +  /* We can now die because easy AI may accidently stumble on huts it fuzzily
               ^^^ This is almost always a wrong word for a comment.

> +  int unit_id = punit->id;

Can be made more local because is has only a limited usage.

> +  /* Desired destination */
>    int best_x = -1, best_y = -1;

best_[xy] is sometimes guared by bestcost and sometimes by
most_unknown. Can't this be improved somehow?

> +      /* We have to be careful with triremes, we won't travel to another
> +       * continent (that means sea units will try to explore only sea, 
> because
> +       * they have continent 0, as all the oceans have), into cities (unless 
> we
> +       * are able to do so - diplomats and caravans can (FIXME/TODO: special
> +       * flag for this)) and we won't raze huts if we are barbarians. */
> +      if (unknown > most_unknown
> +          && (!unit_flag(punit, F_TRIREME)
> +              || trireme_loss_pct(pplayer, x1, y1) == 0)
> +          && map_get_continent(x1, y1) == continent
> +          && can_unit_move_to_tile_with_notify(punit, x1, y1, 0)
> +          && !(map_get_city(x1, y1) && (unit_flag(punit, F_DIPLOMAT) ||
> +                                        unit_flag(punit, F_CARAVAN)))
> +          && !(is_barbarian(pplayer) && map_get_special(x1, y1) & S_HUT)) {
> +        most_unknown = unknown;
> +        best_x = x1;
> +        best_y = y1;

What about splitting this big condition into a series of
  /* alkjsdlkajsdjasdkj */
  if(cond1) {
     continue;
  }

  /* lsekjflskjflksdfkj */
  if(cond2) {
     continue;
  }
  ....

This would it easier to read and document IMHO.

> +      if (!handle_unit_move_request(punit, best_x, best_y, FALSE, FALSE)) {
> +        /* This shouldn't happen */

Than add an assert(0);

> +    /* XXX: There's some duplicate code here, but it's several tiny pieces,
> +     * impossible to group together and not worth their own function
> +     * separately. --pasky */

....

> +        /* Number of unknown tiles in vision range around this tile */
> +        int unknown = 0;
> +        
> +        square_iterate(x1, y1, range, x2, y2) {
> +          if (!map_get_known(x2, y2, pplayer))
> +            unknown++;
> +        } square_iterate_end;

This could be made a function. This construct is used above.

> +
> +      /* XXX: Why we don't test if the unit is still alive? */

Add an assert which test it.

About the "XXX" and "pasky" comments: we need a consistent style for
this. For example we have to decide between FIXMEs and unclear/unknown
purpose.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "SIGDANGER - The System is likely to crash soon"