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

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

At 03:02 PM 01/12/28 -0600, Mike Kaufman wrote:
>On Mon, Dec 24, 2001 at 11:30:28AM +0100, Petr Baudis wrote:
>> Dear diary, on Mon, Dec 24, 2001 at 10:46:36AM CET, I got a letter,
>> where Raahul Kumar <raahul_da_man@xxxxxxxxx> told me, that...
>> > int want = pcity->ai.building_want[acity->currently_building];
>> > +      /* Distance to wonder city was established after manage_bu and
before
>> > +       * this.  If we just started building a wonder during a_c_c_b, the
>> > +       * started_building notify comes equipped with an update.  It
calls
>> > +       * generate_warmap, but this is a lot less warmap generation
than there
>> > +       * would be otherwise. -- Syela */
>> > +      int dist;
>> > 
>> > I have no idea what this line 
>> > 
>> > If we just started building a wonder during a_c_c_b, the started_building
>> > notify comes equipped with an update. 
>> > 
>> > a_c_c_b baffles me. If you think this actually makes else, keep it.
>> Ok, I expanded the function names here :-).
>
>Could I get a couple of reviewers (and reviews) for this patch? Ross,
>Raahul?, anyone else?
>
>-mike
====


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. 

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.

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.

Cheers,
RossW
=====


Index: advdomestic.c
===================================================================
RCS file: /home/cvs/aiciv/freeciv-a2/ai/advdomestic.c,v
retrieving revision 1.1.1.3
retrieving revision 1.15
diff -u -r1.1.1.3 -r1.15
--- 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.
===

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

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

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

-  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.
===
 
+       build_points_left(acity) > get_unit_type(unit_type)->build_cost *
caravans) {

*** Another line wrap.
===

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

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

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

*** I think the code block to localize the struct is both poor style and
    completely unnecessary. It is actually better in "C" to collect local
    variables at the top of the routine where the maintainer can easily
    find them. 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.
    It also adds unnecessary extra indentation.
    Any value in this is more than masked by the drawbacks.
===

+  /* 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.
===

+    /* 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.
===

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