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

[Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx>]

 --- Petr Baudis <pasky@xxxxxxxxxxx> wrote: 
> Dear diary, on Sun, Feb 17, 2002 at 07:58:05PM CET, I got a letter,
> where Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx> told me, that...
> > > > 
> > > > 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]

sure, it is buggy.  maybe you can write a better one?

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

well, think about this: will a unit ever do pillage??
here we are considering moving to a tile (x1,y1) to do pillage.
what happens when we arrive there?  do you think it will remember what it
wanted on this tile?

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

oh well, our opinions differ.

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

well SET_BEST seems more of a kludge to me.
making a new variable (local to iterate loop) 
int new_best=0;

and adding before iterate_end
if (new_best > best) {
  best = new_best;
  dest=...
}

is not a kludge at all.  Raimar?

> > Somehow I like it more.  Don't really know why.
> 
> Let's see now :).

Mmmm, not much better.  Let's see why.


I know I suggested it, but it looks ugly :((
> +#define SET_BEST(new_best) \
> +  do { best = new_best; *dest_x = x1; *dest_y = y1; } while (0)


> +      /* Horsemen in city refused to attack phalanx just outside that was
> +       * bodyguarding catapult; thus, we get the best attacker on the tile
> as
> +       * well, for the case when there are multiple different units on one
> +       * title. Thus we force punit to attack a stack of units if they are
> +       * endangering punit seriously, even if they aren't that weak. */
> +      struct unit *patt = get_attacker(punit, x1, y1);
> +
> +      if (!can_unit_attack_tile(punit, x1, y1))
> +        continue;
> +
> +      /* If we are in the city, let's deeply consider defending it. See
> patt's
> +       * comment for details about the big unequality. */

You call that explanation????
This is just a statement of intent.  Explanation would explain _why_ these
particular values are compared!  If you don't know, just say it (not in the
code though ;).  I don't know.  But I have strong suspicion that this
inequality is _wrong_ :
get_total_defense_power(pdef, punit) * get_total_defense_power(punit, pdef) 
>= 
get_total_attack_power(patt, punit) * get_total_attack_power(punit, pdef)

It should have get_total_defense_power(patt, punit), not (pdef,punit).

> +      if (map_get_city(x, y)
> +          && get_total_defense_power(pdef, punit) *
> +             get_total_defense_power(punit, pdef) >= /* didn't like > --
> Syela */
> +             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) {
> +        freelog(LOG_DEBUG, "%s defending %s from %s's %s",
> +                unit_type(punit)->name,
> +                map_get_city(x, y)->name,
> +                unit_owner(pdef)->name, unit_type(pdef)->name);
> +        

ugh, my head is killing me
time to go to sleep...

__________________________________________________
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