[Freeciv-Dev] Re: advdomestic.c cleanup II. (PR#1157)

[Petr Baudis <pasky@xxxxxxxxxxx>]

Dear diary, on Sun, Dec 30, 2001 at 07:49:49PM CET, I got a letter,
where "Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx> told me, that...
> Subject:   Review of advdomestic.c9.dif
> Submitter: Petr Baudis <pasky@xxxxxxxxxxx>
> Date:      2001/12/29
> Reviewer:  Ross Wetmore <rwetmore@xxxxxxxxxxxx>
> 
> Patched cleanly to CVS-Dec27. 
> Compiles cleanly.
> Runs. Before/after server savegames were different.
> 
> Most problems are tab/line length issues, but this *is* a style not 
> substance patch, and it is important to get such things right. Couldn't 
> spot why the runtime behaviour changed in quick pass. Might be hardcoded 
> vs lookup values for some parameters with a private ruleset, but it is 
> probably not that critical. 
Well, I admit that I tested only the original version of the patch.

> There are a number of spots where FIXME/TODO comments might be inserted
> to mark followup changes. Thus if they aren't made immediately, the info
> is not lost. This should be part of the documentation/discovery process,
> i.e. the cosmetic cleanups should explain, and also raise issues where 
> explanation is not forthcoming. Indications of future directions for code 
> changes make reasonable docs additions to capture the understanding 
> gleaned during docs cleanups and insure that pieces or the codebase are 
> better tied together when the real fixes eventually go in.
Sure.

> No fundamental problems apart from the tabbing. This can go in with
> minimal touchups, though it would be nice to elaborate at a few points.
> Places are generallly marked Out-of-scope, which means not part of the
> submitted patch, but perhaps would be useful suggestions to pursue at
> the submitter's discretion.
Sure.

> --- advdomestic.c     23 Dec 2001 18:17:19 -0000      1.1.1.3
> +++ advdomestic.c     29 Dec 2001 11:55:53 -0000      1.15
> @@ -263,7 +263,11 @@
>    
>    return cost;
>  }
> - 
> +
> +/**************************************************************************
> +Evaluate desirability of all city improvements and fill
> pcity->ai.building_want
> +with them.
> 
> *** Better might be something like ...
> 
> +Evaluate the current desirability of all city improvements for the given
> +city to update pcity->ai.building_want.
Ok.
> ===
> 
> @@ -628,54 +632,68 @@
> 
> -  int est_food = pcity->food_surplus + 2 * pcity->ppl_scientist + 2 *
> pcity->ppl_taxman; 
> +  /* Food surplus assuming that workers and elvii are already accounted for
> +   * and properly balanced. */
> +  int est_food = pcity->food_surplus +
> +                 2 * pcity->ppl_scientist +
> +              2 * pcity->ppl_taxman; 
> 
> *** bad tabs, be consistent
Ok. I will do :set expandtabs next time ;).
> ===
> 
>    choice->choice = 0;
>    choice->want   = 0;
>    choice->type   = CT_NONE;
> 
> *** Out of scope, but the choice struct should be initialized by a macro
>     or call to initialize function, not in scattered private code. This
>     permits it to be updated and improved from a single code location.
>     Use an init_choice(&choice) (best for canned default init) or 
>     set_choice(&choice, args) (GP underlying function) format.
Sure - I plan (of course I won't be angry if someone else will do it ;)
to introduce these functions and fix all these cases in one separate patch,
dedicated to this.
> ===
> 
> +
> +  if (est_food > utype_food_cost(get_unit_type(unit_type), gov)) {
> +    /* founder_want is calculated in settlers.c, called from
> ai_manage_city(). */
> 
> *** The above comment exceeds 80 columns. Remove the "is" that was added.
Ok.
> ===
> 
> -  city_list_iterate(pplayer->cities, acity)
> -    if (acity->is_building_unit && acity->shield_stock >= 50 &&
> -        unit_type_flag(acity->currently_building, F_CARAVAN) &&
> -        map_get_continent(acity->x, acity->y) == con) vans++;
> -  city_list_iterate_end;
> 
> +  /* Count caravans being built */
> +  city_list_iterate(pplayer->cities, acity) {
> +    if (acity->is_building_unit &&
> +        unit_type_flag(acity->currently_building, F_CARAVAN) &&
> +     acity->shield_stock >=
> get_unit_type(acity->currently_building)->build_cost &&
> +        map_get_continent(acity->x, acity->y) == continent) caravans++;
> +  } city_list_iterate_end;
> 
> *** The original line formatting was much better. Clean up the acity line to
>     tab equivalently and not overflow 80 columns.
The original line formatting is impossible to achieve now ;-). And I think
the unit_type_flag() should be near acity->is_building_unit. Rewrapped now.
> ===
>  
> +     build_points_left(acity) > get_unit_type(unit_type)->build_cost *
> caravans) {
> 
> *** Another line wrap.
Ok. Funny that people told me that I shouldn't do those changes I indent
will care about them ;-).
> ===
> 
> +      } else {
> +             /* Had to add the scientist guess here too. -- Syela */
> +     
> +     int tech_req = get_unit_type(unit_type)->tech_requirement;
> +     
> +     pplayer->ai.tech_want[tech_req] += want;
> +      }
> 
> +      } else {
> +     int tech_req = get_unit_type(unit_type)->tech_requirement;
> +     
> +     /* Had to add the scientist guess here too. -- Syela */
> +     pplayer->ai.tech_want[tech_req] += want;
> 
> *** More bad tabbing. I would condense things a bit and assign the comment
>     to the pertinent line.
Ok.
> 
> *** Out of scope. This is really bad monolithic coding that causes bugs
>     which are impossible to detect. There should be *GROSS HACK* warnings
>     and some standard way to pass/save input for the tech advisor for
>     *this* request. Context-free global update from arbitrary code is evil.
Are XXX (FIXME) tags enough?
> ===
>  
> -  ai_advisor_choose_building(pcity, &cur);
> -  if (cur.want > choice->want) {
> -    choice->choice = cur.choice;
> -    choice->want = cur.want;
> -/*    if (choice->want > 100) choice->want = 100; */
> -/* want > 100 means BUY RIGHT NOW */
> -    choice->type = CT_BUILDING;
> -  }
> -/* allowing buy of peaceful units after much testing -- Syela */
> -
> -  if (!choice->want) { /* oh dear, better think of something! */
> -    iunit = best_role_unit(pcity, F_CARAVAN);
> -    if (iunit == U_LAST) {
> -      iunit = best_role_unit(pcity, F_DIPLOMAT);
> -      /* someday, real diplomat code will be here! */
> +  {
> +    struct ai_choice cur;
> +    
> +    ai_advisor_choose_building(pcity, &cur);
> +    copy_if_better_choice(&cur, choice);
> +    
> +    /* Allowing buy of peaceful units after much testing. -- Syela */
> +    /* want > 100 means BUY RIGHT NOW */
> +    /* if (choice->want > 100) choice->want = 100; */
> +  }
> 
> *** Out-of-scope: copy_if_better() is the wrong direction. 
>     The general rule is that instance/target/return values come first and 
>     any (readonly) arguments are strung out afterwards. 
>     This follows the "C++" flavour where the "instance" pointer is the 0'th 
>     parameter.
>     This should be the norm *everywhere* in the code so lines like the
>     above are unambiguous, and *not* subject to transposition errors.
Ok. The order of parameters should be changed in choice cleanup.
> 
> *** Nit. The commented out if clause should really only be applied to the
>     new calculated choice and not the "current" value, i.e. the comments
>     should be placed above the copy_if_better(). This changes the original
>     but it was IMHO wrong, since any such fixes would already have been
>     applied to the "current" and should not be reapplied here. If you read
>     the original comment and strictly interpret "units", then this change
>     is required, as "buildings" are not units.
Ok. Wasn't really sure about this.
> 
> *** I think the code block to localize the struct is both poor style and
>     completely unnecessary.
I don't think so and I have a feeling that localization of the variables
is a common trend in FreeCiv (and I like it).
>                             It is actually better in "C" to collect local
>     variables at the top of the routine where the maintainer can easily
>     find them.
Both maintainer and random reader of the core will probably look for them
near their usage.
>                In "C" one doesn't generally expect embedded variable
>     declarations, and in any lengthy code block this would be a problem,
>     especially if the same variable were defined in more than one block.
If the variable is declared inside some special block, I think it's very
clear (i.e. thanks to indendation), in which scope the variable is declared
and used, and it's also easy to see how it was declared and initialized.
>     It also adds unnecessary extra indentation.
It tells you that this code is something separated, which does some kinda
'subfunction', but it's not worth put it to special function ;).
>     Any value in this is more than masked by the drawbacks.
I don't agree.
> ===
> 
> +  /* FIXME: rather (choice->want <= 0) --rwetmore */
> +  if (!choice->want) {
> 
> *** If you want to expand on this, negative choice->wants are typically
>     symptoms of something gone haywire and should not be allowed out of
>     this routine. But there are some hack-ceptions to this rule as with 
>     ferryboats.
>     Better, miught be a RANGE_CHECK or is_valid_choice() for valid want 
>     range. Raimar would suggest the really bozo cases should be an assert 
>     as well.
Ok, I will change this ;).
> ===
> 
> +    /* Oh dear, better think of something! */
> +    unit_type = best_role_unit(pcity, F_CARAVAN);
> +    
> +    if (unit_type == U_LAST) {
> +      unit_type = best_role_unit(pcity, F_DIPLOMAT);
> +      /* Someday, real diplomat code will be here! */
>      }
> -    if (iunit != U_LAST) {
> +    
> +    if (unit_type != U_LAST) {
> 
> *** Out-of-scope: these should really be RANGE_CHECKs for allowed unit
>     type rather than just fall-off-the-end tests.
> ===
> 
>        choice->want = 1;
>        choice->type = CT_NONMIL;
> -      choice->choice = iunit;
> +      choice->choice = unit_type;
> 
> *** Out-of-scope: Use a set_choice(&choice, args) format. Macro is fine
>     but a function might be used when it gets complicated. This allows
>     one to extend the choice struct and update all uses with default
>     values with just one code change.
Sure.
> ===
> 
>      }
>    }
> -  if (choice->want >= 200) choice->want = 199; /* otherwise we buy
> caravans in
> -city X when we should be saving money to buy defenses for city Y. -- Syela */
> +  
> +  /* If we don't do following, we buy caravans in city X when we should be
> +   * saving money to buy defenses for city Y. -- Syela */
> +  if (choice->want >= 200) choice->want = 199;
> 
> *** This isn't really much of an improvement. Find out where/why 200+ values
>     do something that 199's don't and explain in detail here. I prefer the
>     "otherwise" flavour, otherwise. It suggests an incomplete addendum
>     rather than purporting to be some sort of respectable explanation.
This is IMHO rather a subject for common explanation of want values.

-- 

                                Petr "Pasky" Baudis

UN*X programmer, UN*X administrator, hobbies = IPv6, IRC, FreeCiv, (e)links
.
Firewall in a way is like the doorkeeper of a local pub. If you don't have your
I.D. on you, or if for some reason you do not qualify to enter, the doorkeeper
will not permit you to enter. In some extreme cases these doorkeepers will not
let you out, or at least give you a hard time before they finally let you out.
.
Public PGP key, geekcode and stuff: http://pasky.ji.cz/~pasky/