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

[Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx>]

 --- Petr Baudis <pasky@xxxxxxxxxxx> wrote: 
> Dear diary, on Sat, Feb 16, 2002 at 09:29:21PM CET, I got a letter,
> where Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx> told me, that...
> > 
> > you should probably explain the flow of the function here too
> 
> Huh? Like?

Mmmm...  Forget it...
It's good enough.

> > > -    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?

> > > +    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...

> > > -/* 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) {

[...]

> > I think "health" part of your comment is wrong.
> 
> Can you elaborate, please? Why it is wrong, what would be more proper
> interpretation of that formula etc.

No maybe it is right...
Weird...

> > 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

> > 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.

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) ?
Or, even better, change *dest_x and *dest_y in some centralised place,
not every time you change best.  Yes, that's it.

> > It can be done (1) in this patch, (2) before this patch or (3) after
> this
> > patch.
> 
> In this patch or after this patch! :^) This patch should already at

I also think so.

> Sure. Can you be please more concrete?

Well, it seems only defend-or-not-defend inequality lacks documentation.

> Fixed patch attached.

Somehow I like it more.  Don't really know why.
BTW, there is still one apetency lurking...

Best,
G.


__________________________________________________
Do You Yahoo!?
Everything you'll ever need on one web page
from News and Sport to Email and Music Charts
http://uk.my.yahoo.com