[Freeciv-Dev] Re: [PATCH] directional code cleanup: patch #1

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Aug 15, 2001 at 03:28:26AM -0400, Jason Dorje Short wrote:
> Trent Piepho wrote:
> > 
> > On Wed, 15 Aug 2001, Jason Dorje Short wrote:
> > > This patch removes the magic numbers and expressions associated with the
> > > DIR_D[XY] macros, collecting all such implementation-dependent code into
> > > map.[ch].
> > 
> > I see that it doesn't change the directions, so there should be fewer
> > possibilities for bugs.  But if you changed them, then any missing magic 
> > bits
> > would be easier to find.  Of course, maybe you already did that when you
> > looked for things to change.
> 
> The directions remain the same (for now)...the patch fixes things up
> without any controversy (the behavior should be identical).
> 
> Yes, I have tried things out with different directions (changing the
> macros as necessary).  Everything seems ok.
> 
> > > Additions to map.[ch]:
> > >  - macros dir_reverse(dir) and dir_is_cardinal(dir) are created
> > 
> > Since these are macros, convention is to give them names in all caps.  
> > Perhaps
> > shorter names..
> 
> They are macros, but act just like functions (and could be made so in
> the future).  The dir_ prefix was recommended - reverse and is_cardinal
> seem like the most logical names.

I would like to second Trent here. There are macros and this property
should be visible by the name. You may also define both: a macro and a
function.

> > >  - the directional names used for logging ("N") are defined globally
> > 
> > Wouldn't a function be better than a global variable?

I also thought about this when I read the patch.

> To make it easier to internationalize?  

No.

> These strings are only used in debugging output, and should not be
> translated.  I don't see any other advantage of a function - you
> might as well use a function in place of DIR_DX and DIR_DY.  Am I
> missing something?

Compare 
  freelog(LOG_NORMAL, "direction of next step is %s",DIR_NAMES[dir]);
with
  freelog(LOG_NORMAL, "direction of next step is %s",get_dir_name(dir));

At least for me the second one looks nicer. And you can also put extra
checks in the function. So I would propose something like this:

const char *get_dir_name(int dir)
{
   assert(dir>=0 && dir<8);
   return DIR_NAMES[dir];
}

> > I think this is a pretty good idea.  The directions are really a mess they 
> > way
> > they are now.

Ack.

> Well, there's another mess here.  The client defines its own set of
> cardinal directions, using an enumeration and the names DIR_NORTH, etc. 
> There will inevitably be confusion.  I made a comment about this in my
> own enumeration.

After this one gets committed I hope I will see a second patch from
you.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Reality? That's where the pizza delivery guy comes from!"