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

[Petr Baudis <pasky@xxxxxxxxxxx>]

Dear diary, on Tue, Jan 08, 2002 at 05:55:20PM CET, I got a letter, where
Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx> told me, that...
> 1. The vision range should NOT take into account watchtower benefit.  Now it
> is done in a wrong way: the watchtower bonus is computed locally, at the
> point where the unit is, and then it is applied at a faraway location, as if
> there is a watchtower there too.  To make it completely correct, you should
> make a separate function unknown_tiles_around(int x, int y, unit *punit)
> which 
> 
>  a. determines unit's vision range at (x,y) -- if the player has no info on
>  (x,y), assume there is no watchtower
> 
>  b. count the number of unknown location within the vision range.  This
>  function would be used at least in 2 places, so it's worth it.
And it's worth separate patch. I don't want to get my patch too big. Are you
willing to make it?

> 2. Comments like
> > +  /* The unit's owner */
> >    struct player *pplayer = unit_owner(punit);
> are completely useles IMO, as the function name says the same thing.
There were some notes about commenting etc ;p. I wanted to see how many
people will catch this and throw it on me - you were the only, quite funny ;).

> 3. Splitting of long if (...) condition into many if() continue;
> is nice.  There is another place that you can do the same.
Which one?
> 
> <ASIDE>
> I really prefer statements like
>       if (!points_left) continue;
> to 
>       if (points_left) {
>         /* very long block */
>       }
> as it explicitly says that there will be no "else" 234 lines down the
> code, it also reduces indentation which is sometimes hard to trace.
> Shall we include it in the style guide?
> </ASIDE>
True, but I wouldn't put this into the style guide - it can vary from
case to case.

> 4. do_unit_goto now returns a meaningful value, you can check for it instead
> of using unit.id
Thanks! :) I disliked this unit_id stuff a lot. Too bad we can't do the same
with handle_unit_goto_request(). Also added some TODO for taking advantage from
more do_unit_goto() results.

> 5. you can move the first generate_warmap call inside the if (looks_for_huts)
> block for a slight improve in efficiency.
Ok. Same in PART 3 (no efficiency gain but keeped consistency :).

Improved patch attached. I also added symbolic constant for the '9' in part 3
move cost applying to unknown. I would still like to get some more explanation
of this, if anyone has an idea.

-- 

                                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