[Freeciv-Dev] Re: [PATCH] cleanup to canvas_pos_to_map_pos (PR#1183)

[Chris Richards <chrisr@xxxxxxxxxxxxxxxx>]

Once upon a while I came across a metrics program that objectively
(well, objectively as their programmers thought what was objective)
and it measured code readability by counting lines of code, number of
operations per line, etc.

The "readability" factor was better with less operations per line even
though the line count was higher.  It also enhanced the readability
score when parentheses were used when not required (they used a list
of commonly confused order-of-operations).  The ternary operator was
bad-bad though I love it to death.  Lines of code with comments before
them also scored well (though one could argue that wrong comments are
worse than no comments).  Code that didn't do anything but relied on
its side effects were shunned, too.

I think the main reason it measured this way was an attempt to help
maintainers, often junior programmers, maintain the code easier.  They
thought it better to understand what the code was doing rather than
trying to think about/distill what was written.

For example, from combat.c:

  double att_P_lose1 = (as + ds == 0) ? 0.5 : (double) ds / (as + ds);

would be given a low score.  Many operations.

Another example, from plrhand.c:

 pplayer1->diplstates[player2].type
    = pplayer2->diplstates[player1].type
    = pplayer1->ai.control || pplayer2->ai.control ? DS_WAR : DS_NEUTRAL;

would be given a low score because of a potentially confusing order of
operation and a ternary operator.  Most the people we interview can't
answer something like this.  Heck, most people we interview can't
manage pointers to pointers, either.

Granted, both of these examples are rather mild in comparison to some
of the more arcane code out there ("See how clever I am!" exclaims
Bob).

In a round about way, I have gotten to my point.  You need to decide
on the audience of code consumers and attempt to code to that
standard.  However, I think that with open source projects you tend to
get a lot of egos that like to do "fancy" coding.  The more complex
and terse a code fragment is, the better.

Personally, I am fine with the current code base (save the arcane
variable names--geez).  But since you were discussing it, I thought I
would throw this mail out.

And, BTW, if conditional tests need to be line split, it should be:

  if (foo->bar
      && bar->foo)

*grin*
      
Cheers,
/cjr