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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Nov 30, 2001 at 11:16:45PM +0100, Petr Baudis wrote:
> > 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.

I think indent will cover this.

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

Then please take a look.

> , I used it only for decryption of IOCCC winners yet ;-).

> And if you will run it thru indent anyway...

I run the parts which are changed thru indent.

> > 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 ;-).

I'm for C.

> > 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 you may have notice I prefer longer names. So I would prefer
"unit_type" but I think we should try to have discussion about this
(with other people).

> > 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?

Remember: we (well I) want review from others -> to freeciv-dev.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "These download files are in Microsoft Word 6.0 format. After
  unzipping, these files can be viewed in any text editor, including
  all versions of Microsoft Word, WordPad, and Microsoft Word Viewer."
    -- http://www.microsoft.com/hwdev/pc99.htm