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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Feb 20, 2002 at 10:18:19PM +0100, Petr Baudis wrote:
> Dear diary, on Wed, Feb 20, 2002 at 09:49:53PM CET, I got a letter,
> where Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> told me, that...
> > > > > +        SET_BEST(99999);
> > > > ...
> > > > > +      if (map_has_special(x1, y1, S_HUT) && best < 99999
> > > > > +          && could_unit_move_to_tile(punit, punit->x, punit->y, x1, 
> > > > > y1) != 0
> > > > > +          && !is_barbarian(unit_owner(punit))
> > > > > +          && punit->ai.ai_role != AIUNIT_ESCORT
> > > > > +          && punit->ai.charge == 0 /* Above line doesn't seem to 
> > > > > work. :( */
> > > > > +          && punit->ai.ai_role != AIUNIT_DEFEND_HOME) {
> > > > > +        SET_BEST(99998);
> > > > > +      }
> > > > 
> > > > These 9999* are ugly.
> > > 
> > > And what shall we do with them..?
> > 
> > Change the SET_BEST to a SET_BEST_IF_BETTER (this will remove the
> > "best < 99999"). Add
> > 
> > #define BEST_FITNESS 9999
> > #define SECOND_BEST_FITNESS 9998

> I don't like this. I want to keep SET_TARGET as universal as possible.

This macro can only be used for this function. In the next one the
coordinates are maybe named x,y or x2,y2 instead of x1,y1. If C would
allow nested function this could be solved in a nice way. C doesn't
have it and so we are forced to either use macros or create extra
functions (which maybe need a lot of arguments).

> And I don't think thas those 99999 constants deserve their own
> #define. It is used only twice on two very near places on the code,
> it's not something you would want to change frequently, its
> mean/purpose should be pretty clear, and after all the symbolic
> names are terribly ugly ;). And as they don't really explain the
> value you will anyway scroll up to see what they are defined as when
> you'll read the code.

If you don't want defines that put a comment in the header which says
something like this:
 9999 is used to denote the best case we can achieve (empty city)
 9998 ....
 all other values are in the range of... (*looking at the code* mhhh
 could it be that the set of values is only - 2 * MORT *
 TRADE_WEIGHTING, 0, 1, 99998 and 99999)?

> > > +  if (punit->unhappiness > 0) {
> > > +    best = 0 - 2 * MORT * TRADE_WEIGHTING; /* desperation */
> > > +  }
> > 
> > Can you explain this one?
> 
> Sorry, but I think I can't :-(. I know no logic argument why we shouldn't
> attack with units causing unhappiness, thus apparently this looks like some
> interesting sociologic experiment/simulation ;).

no comment

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Premature optimization is the root of all evil."
    -- D. E. Knuth in "Structured Programming with go to Statements"