[Freeciv-Dev] Re: AI cleanup - first wave

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Nov 30, 2001 at 08:58:55PM +0100, Petr Baudis wrote:
> Hello,
> 
>   I want to tell you that I'm done with the possibly largest part of AI code
> cleanup. Formatting changed, the code was heavily commented, some unused
> functions were removed, some other were moved (still some queued) and the code
> was simplified in few places. However the AI behaviour should remain same for
> now, and it did according to my autogame tests.
> 
>   In which way I should feed you the patches? I have them in my private CVS
> tree accessible thru cvsweb 
> (http://pasky.ji.cz/cvsweb.cgi/freeciv/ai/?cvsroot=aiciv)
> and if you want i have no problem with giving you direct access to the CVS
> itself.  It would be maybe most convient for both you and me to get the 
> patches
> thru CVS (mostly one function == one commit), however some functions are
> corrected by later revisions, so if you won't will to complete the correct
> patches for those, i can do it. Also, if you are not willing to check the
> changes out of my CVS, i can feed you with patches thru mail, but there will 
> be
> many of them and not everytime smallest ones (for large functions large
> patches, i even had to split two additional functions from ai_eval_buildings()
> as it was way tooo large).
> 
>   I would also like to ask others to review my version of AI and review it,
> especially Tony to check my changes in his code ;-). Note that aiunit.c and
> advmilitary.c are still mostly uncleaned (only patches I posted separately
> before are commited), as I want to work those changes out together with 
> Raahul.
> 
> Many thanks for comments,

I think a requirement should be that someone else go through the
patches. How are these changes related to the ongoing style
discussion? I also noticed that you made changes which is the job of
indent. Like this

-    }
-    else {
+    } else {

Another time for the "feed all code through indent" discussion?!

As for the style: we have to add another question:

 A
  if (is_barbarian(pplayer)) {
     ....
  /* If not barbarian, ... */
  } else {

 B
  if (is_barbarian(pplayer)) {
     ....
  } else { /* If not barbarian, ... */

 C
  if (is_barbarian(pplayer)) {
     ....
  } else {
    /* If not barbarian, ... */

As you renamed variables we may have to also setup a style guide on
this, since I'm not very happy with the variable naming in this
example (besides this it is a huge improvement):

-    for(i = 0; i < num_role_units(L_BARBARIAN_BUILD); i++) {
-      iunit = get_role_unit(L_BARBARIAN_BUILD, i);
-      if (get_unit_type(iunit)->attack_strength > bestattack) {
-       bestunit = iunit;
-       bestattack = get_unit_type(iunit)->attack_strength;
+
+    for (role = 0; role < num_role_units(L_BARBARIAN_BUILD); role++) {
+      Unit_Type_id unit = get_role_unit(L_BARBARIAN_BUILD, role);
       ^^^^^^^^^^^^^^^^^
+
+      if (get_unit_type(unit)->attack_strength > bestattack) {
+       bestunit = unit;
+       bestattack = get_unit_type(unit)->attack_strength;

IMHO this shouldn't be named "unit" but "unit_type" or "utype" or "type".

As for the question you have asked: we should try one function at a
time and see how this works.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  Tank: So what do you need? Besides a miracle.
  Neo: Guns. Lots of guns.
    -- From The Matrix