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

[Petr Baudis <pasky@xxxxxxxxxxx>]

Dear diary, on Sun, Jan 06, 2002 at 11:54:22PM CET, I got a letter,
where Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> told me, that...
> On Sun, Jan 06, 2002 at 01:59:40PM -0800, Petr Baudis wrote:
> >  /**************************************************************************
> > -Explores unknown territory, finds huts.
> > +Handle eXplore mode of a unit - explores unknown territory, finds huts.
>            ^ This is a client-only gui thing
It helps you to associate its function with something. I can lowercase the X,
if you want.

> > +  /* The position of the unit; updated inside the function */
> > +  int x, y;
> 
> Since also unit_id is initialized here x and y can also.
Can't it change with handle_unit_activity_request()? (no idea)
> 
> > +  /* We can now die because easy AI may accidently stumble on huts it 
> > fuzzily
>                ^^^ This is almost always a wrong word for a comment.
Why? (removed it anyway ;)
> 
> > +  int unit_id = punit->id;
> 
> Can be made more local because is has only a limited usage.
Ok.
> 
> > +  /* 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?
IMHO nope. They measure different things so they should be named differently.
But I made them local :).
> 
> > +      /* 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.
Nice idea. Done. I also removed almost all comments as it's pretty clear what
we consider now. If someone think some of the ifs is worth a comment, tell me 
:).
> 
> > +      if (!handle_unit_move_request(punit, best_x, best_y, FALSE, FALSE)) {
> > +        /* This shouldn't happen */
> 
> Than add an assert(0);
There's no "this can't happen" - if it happens, it is still ok, we aren't
assured anywhere it won't happen. See source of handle_unit_move_request() and
you will see really many cases when return 0 is issued and most of them aren't
fatal. I.e.:

    /* DO NOT Auto-attack.  Findvictim routine will decide if we should. */
    if (pplayer->ai.control && punit->activity == ACTIVITY_GOTO) {
      notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT,
                       _("Game: Aborting GOTO for AI attack procedures."));
      return 0;
    }

When I observe AI, I see this frequently. IMHO we would die really soon with
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.
I think it's too small - and we do it only two times and we have a space for
it. And we would have to pass too many parameters to it (x1, y1, range,
pplayer, &unknown)... not worth it, IMHO.
> 
> > +
> > +      /* XXX: Why we don't test if the unit is still alive? */
> 
> Add an assert which test it.
Well, when some unit gets killed during goto, we don't want to victimize whole
civserver for this one poor fellow duteous to its honourable mission of
exploring of those white (erm, black) areas of the map bedight by whales and
notes like "hic sunt liones". Sure, maybe for second all the daemon (the guys
at the sides blowing) processes should stop and for the eternal glory of it all
buffers should be synced, but IMHO those great fellows serving loyally to Karel
IV. of Czech wouldn't wish themselves to bury all the world with them, just for
one assert().
> 
> 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.
I have consistent (I hope :) style for this ;-). I sign myself near my personal
opinions/questions. XXX means unclarity/glitch in the code, FIXME means it's
actually even confirmed as buggy or confirmed that it needs to be replaced.
Yup, there's no sharp boundary between these two, but hey, we aren't machines.

Updated patch attached. Compiles ;-).

I'm now not sure if ai_manage_explore() wouldn't be even more appropriate. What
you guys think?

-- 

                                Petr "Pasky" Baudis

UN*X programmer, UN*X administrator, hobbies = IPv6, IRC, FreeCiv, (e)links
.
The advantage of GUI is that you can see everything you can change.
The disadvantage of GUI is that you can change only what you can see.
.
Public PGP key, geekcode and stuff: http://pasky.ji.cz/~pasky/

exploring-cleanup.patch
Description: Text document