[Freeciv-Dev] Re: [PATCH] is_normal_tile function

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

It can be done twice here, and that cuts the body of this function in
half. Although going overboard with comments on trivial or prototype 
code can be as bad, a comment here or in the one (maybe two) other
places where this should be defined sounds like a good idea in any
final CVS commit/production version .

I'd actually do a single macro IS_NORMAL(BASE,POS) and explain it, then 
replace all the x and y specific instances with this one everywhere. It
is sort of what is already in map.h.

But you have localized the main use to a call with both x, and y
arguments which is really the only API you want to promote except
in the underlying implementation. So, there shouldn't be that many
cases to consider, and the extra step is overkill unless you already 
had this in the header.

Expanded code may look nice, but having variants on the same code 287 
times in the code base means that cleaning it up or changing it when the 
range is not a simple linear one will be as much of a pain as some of the
current cleanups.

Ask yourself what an is_normal_tile would be for a doubly indexed "set".

And I am surprised that unsigned vs signed compares aren't as obvious 
a "C" technique as they seem to be :-).

Cheers,
RossW
=====

At 10:48 PM 01/08/15 +0200, Raimar Falke wrote:
>On Wed, Aug 15, 2001 at 09:41:17PM +0100, Gregory Berkolaiko wrote:
>>  --- Jason Dorje Short <jshort@xxxxxxxxxxxxx> wrote: 
>> > +***************************************************************/
>> > +int is_normal_tile(int x, int y)
>> > +{
>> > +  return 0 <= y && y < map.ysize && 0 <= x && x < map.xsize;
>> > +}
>> 
>> you might want to use Ross' geeky way of writing such things:
>> 
>> return ((unsigned) y < (unsigned) map.ysize) && (....)
>> 
>> or am I as usual missing something?
>
>It is interesting to see that even a simple function can be
>obfuscated.
>
>       Raimar
>-- 
> email: rf13@xxxxxxxxxxxxxxxxx
>  "Heuer's Law: Any feature is a bug unless it can be turned off."