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

[Petr Baudis <pasky@xxxxxxxxxxx>]

Dear diary, on Sun, Feb 17, 2002 at 07:58:05PM CET, I got a letter,
where Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx> told me, that...
>  --- Petr Baudis <pasky@xxxxxxxxxxx> wrote: 
> > > > -    unit_list_iterate(map_get_tile(x, y)->units, aunit)
> > > > -      if (!is_sailing_unit(aunit)) return(0);
> > > > -    unit_list_iterate_end;
> > > > -  } /* ferryboats do not attack.  no. -- Syela */
> > > 
> > > Is there no better way to say if a boat has passangers.
> > > If the answer is "no" then you should write it, and separate the code
> > > below into it.
> > 
> > Hmm.. after all, shouldn't we just exit when we're a ferryboat,
> > independently
> > on our charge? Why to waste them on the enemies? (if answer is "yes, we
> 
> Why not?  What if somebody set transporter capacity of a battleship to 3?

Perfectly valid point. Let's have a looook... <hours are passing ;>...  Well,
how are represented the passengers of a ferry? The actual code appears as buggy
to me, as it produces wrong results when there're more boats at one tile, one
loaded and one empty (and we're the empty one).  [NOT FIXED]

> > > > +    unit_list_iterate(map_get_tile(x, y)->units, aunit) {
> > > > +      if (!is_sailing_unit(aunit)) return 0;
> > > > +    } unit_list_iterate_end;
> > > > +  }
> 
> [...]
> 
> > > >      if (pdef) {
> > > > -      patt = get_attacker(punit, x1, y1);
> > > 
> > > you remove this comment but don't explain why patt should be used.
> > > bad...
> > 
> > True. Comment added.
> 
> I suspect the idea of patt is to force punit to attack a stack of units
> if they are endangering punit, even if they are not that weak...

True obviously.

> > > > -/* horsemen in city refused to attack phalanx just outside that
> > was
> > > > -bodyguarding catapult - patt will resolve this bug nicely -- Syela
> > */
> 
> [...]
> 
> 
> > > > +      if (!can_unit_attack_tile(punit, x1, y1))
> > > > +        continue;
> > > > +
> > > > +      /* If we are in the city, let's deeply consider defending
> > it. */
> > > > +      if (map_get_city(x, y)
> > > 
> > > what is the meaning of the inequality?
> > 
> > You mean !can_unit_attack_tile() ? It's just intended to remove
> > completely
> > unneccessary indentation level... IIRC it was you who suggested doing
> > this.
> 
> nah, that I like.  What is the meaning of the BIG inequality below:
> 
> > > > +          && get_total_defense_power(pdef, punit) *
> > > > +             get_total_defense_power(punit, pdef) >=
> > > > +             get_total_attack_power(patt, punit) *
> > > > +             get_total_attack_power(punit, pdef)
> > > > +          && unit_list_size(&(map_get_tile(punit->x,
> > > > punit->y)->units)) < 2
> > > > +          && get_total_attack_power(patt, punit) > 0) {

Why do I think my english codec was broken much more than usual yesterday?

> > > How about writing a separate function 
> > > consider_pillaging(x, y, punit) ?
> > > in general, not only barbarians should pillage.
> > 
> > Is it used anywhere else?
> 
> could be but I don't think so.
> but it shouldn't stop you IMO

I don't see why some other code should be considering pillage when we're doing
so.. thus why to move this to separate function. The actual function is not
_that_ long and this may pretty well worse its readability.

> > > Verdict:  The patch improves the readability of the code very much.
> > > However I believe that a lot of code should be separated into smaller
> > > functions.
> > 
> > Please elaborate.
> 
> I think we should make many small functions with good names, like
> consider_defending_town, consider_pillaging, consider_hut_poaching and
> has_passangers.
> 
> Even if these functions are only used once, they will be easier to
> maintain, they will encourage re-use in other parts of AI and they will
> shorten long routines.

This routine is not long. 150 lines, with at least 1/4 comments, is not so long
comparing to many other routines in AI.

I don't see _why_ they should be reused in other parts (except has_passengers),
when they are used here.

Thus the maintenance of them will be always accompanied with maintenance of the
whole function, possibly making it even harder when separating this functions
away.

It's not a bad thing, but I don't think it's worthwhile doing now.

> But this is my philosophy which might be different from the mainstream.
> 
> Also, how about writing local macro SET_BEST(new_best, new_x, new_y) ?

Good idea, done.

> Or, even better, change *dest_x and *dest_y in some centralised place,
> not every time you change best.  Yes, that's it.

How would you imagine that..? Saving old best value and comparing it? That
seems like ugly kludge to me.

> > Fixed patch attached.
> 
> Somehow I like it more.  Don't really know why.

Let's see now :).

> BTW, there is still one apetency lurking...

Whops ;).

-- 

                                Petr "Pasky" Baudis

* elinks maintainer                * IPv6 guy (XS26 co-coordinator)
* IRCnet operator                  * FreeCiv AI hacker
.
No one can feel as helpless as the owner of a sick goldfish.
.
Public PGP key && geekcode && homepage: http://pasky.ji.cz/~pasky/

findvictim-cleanup.patch
Description: Text document