[Freeciv-Dev] Re: [PATCH] aiunit.c ai_manage_explorer cleanup (PR#1210)

[Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx>]

 --- "Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx> wrote: 
> At 08:55 AM 02/01/08 -0800, Gregory Berkolaiko wrote:
> >Few comments on the patch.
> >
> >3. Splitting of long if (...) condition into many if() continue;
> >is nice.  There is another place that you can do the same.
> >
> ><ASIDE>
> >I really prefer statements like
> >     if (!points_left) continue;
> >to 
> >     if (points_left) {
> >       /* very long block */
> >     }
> >as it explicitly says that there will be no "else" 234 lines down the
> >code, it also reduces indentation which is sometimes hard to trace.
> >Shall we include it in the style guide?
> ></ASIDE>
> 
> I'm not unhappy with this particular example, but generally dislike
> this sort of construct in most settings. I am sure there are those 
> who will find all sorts of means to pervert this in ways to destroy
> useful coding patterns.
> 
> Complex conditionals are to be avoided where reasonable, but it is
> usually better to work out the logic of multiple conditions than 
> duplicate the code or access it with a maze of explicit gotos.
> 
> This is almost a goto construct which is why it is tricky to manage
> and easy to abuse.
> 
> If you make the continue; a return; it tends to make functions very
> inscrutable. They should only have one return point. 

There is truth in what you say.  But let's take an example:
function find_something_to_kill, cycle starting at
http://www.freeciv.org/lxr/source/ai/aiunit.c?v=cvs#L1254

Two lines below there is an "if":

1256     

where does it end?

scroll down... and down... and down...

1427     } /* end if enemy */

no else clause, but throughout 170 lines you keep expecting it.
In this situation 

if (aplayer == pplayer) {
  /* we only attack our enemies */
  continue;
}

would be so much better.

Of course the code blocks of such length just should not exist.
And any patches that reduce the length by introducing calls to short
functions should be welcome IMO.  In reality they get 10s of nitpick
comments and get blissfully forgotten about.  

As usual I start rambling about the sad fate of my patches, hehe ;)

> 
> It is almost always useful to have the code guarded by a condition 
> within and bracketted by the if-clause, rather than disconnected from
> it.
> 
> But, I tend to order things to keep the else in visible range of the 
> if as far as possible in dual block cases, and avoid indenting if
> reasonable.
> 
> It would be good to maybe have suggested, style guides with a weight
> that gives some latitude for greyness in application, or else the
> guideline should be limited to super-majority concensus cases of abuse 
> only :-).

Fuzzy rules :)

G.

__________________________________________________
Do You Yahoo!?
Everything you'll ever need on one web page
from News and Sport to Email and Music Charts
http://uk.my.yahoo.com