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

[Petr Baudis <pasky@xxxxxxxxxxx>]

> I think a requirement should be that someone else go through the
> patches.
/me calls someone very loudly...

>          How are these changes related to the ongoing style
> discussion?
I tried to follow the opinion of majority. Now in the cleaned code it is much
more easy to change anything, so almost everyone else can adjust it by our
latest opinion.

>             I also noticed that you made changes which is the job of
> indent. Like this
> 
> -    }
> -    else {
> +    } else {
This is not so clear as it looks. Most times this is obviously better, but
consider this:

      /* if .... */
      else if (....) {
        ...
      }

#ifdef SOMEIMPROVEMENTWHICHNOONEACTUALLYKNOWSITWASABOUT
      
...
#endif

      /* or if .... */
      else if (...) {
        ...
      }

      /* or even if .... */
      else if (...)
        /* do nothing */;

      /* or if .... */
      else if (...) {
        ...
      }

It is prolly better to do }\nelse if here.

> Another time for the "feed all code through indent" discussion?!
Oh, I've never passed any yet! ;-) Anyway, I've no clue what indent exactly
does and what doesn't, I used it only for decryption of IOCCC winners yet ;-).
And if you will run it thru indent anyway...

> As for the style: we have to add another question:
Yeah! Practice shows best what rules we need.
> 
>  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, ... */
I vote for A ;-).

> 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".
Well, I wanted it to be something descriptive, nts, not_too_long, however I
didn't succeed much here, it looks ;-). I will change it to utype, thanks.


> As for the question you have asked: we should try one function at a time and
> see how this works.
Ok, should I feed it to you or you will dig it by yourself after we will tune
it?

-- 

                                Petr "Pasky" Baudis

UN*X programmer, UN*X administrator, hobbies = IPv6, IRC, FreeCiv hacking
.
  "A common mistake that people make, when trying to design
   something completely foolproof is to underestimate the
   ingenuity of complete fools."
     -- Douglas Adams in Mostly Harmless
.
Public PGP key, geekcode and stuff: http://pasky.ji.cz/~pasky/