[Freeciv-Dev] Re: [PATCH] more small directional cleanups

[Jason Dorje Short <jshort@xxxxxxxxxxxxx>]

Trent Piepho wrote:
> 
> On Mon, 20 Aug 2001, Jason Dorje Short wrote:
> > I added an assertion to make sure the loop terminates (if debugging;
> > otherwise we assume it terminates).  Bailing out won't really do any
> > good since it would leave the game in an unstable state.
> 
> Here are I think better ways to do it.  My rand_neighbor function will run in
> a bounded time, and will aways find a neighbor if one exists.

Clever.  I like it.  Although someone pointed out that this should be
"unnecessary", the comfort of knowing that it's safe is well worth it.

> This is I think a little nicer way to do dir_get_name().
> 
> const char *dir_get_name(enum direction8 dir)
> {
>   /* a switch statement is used so the ordering can be changed easily */
>   switch(dir) {
>     case DIR8_NORTH:     return "N";
>     case DIR8_NORTHEAST: return "NE";
>     case DIR8_EAST:      return "E";
>     case DIR8_SOUTHEAST: return "SE";
>     case DIR8_SOUTH:     return "S";
>     case DIR8_SOUTHWEST: return "SW";
>     case DIR8_WEST:      return "W";
>     default:
>       assert(0);
>       return "Bad Direction";
>   }
> }

Better because it's in the "default" case or better because it returns
"Bad Direction"?  The second is good, the first one I find less legible.

Patch attached.  Note that this still leaves magic numbers in
client/tilespec.c (as Ross pointed out), but they're associated with the
DIR_DX2 directional system so they can't be integrated with the DIR_DX
system.

jason

Index: common/map.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/map.c,v
retrieving revision 1.80
diff -u -r1.80 map.c
--- common/map.c        2001/08/20 07:56:01     1.80
+++ common/map.c        2001/08/22 03:33:17
@@ -1318,21 +1318,26 @@
 **************************************************************************/
 void rand_neighbour(int x0, int y0, int *x, int *y)
 {
-  int choice;
+  int n;
+  int dirs[8] = {0, 1, 2, 3, 4, 5, 6, 7}; /* list of all 8 directions */
+  assert(is_real_tile(x0, y0));
 
-  if (y0 == 0) {
-    choice = 3 + myrand(5);
-  } else if(y0 == map.ysize-1){
-    choice = myrand(5);
-  } else {
-    choice = myrand(8);
-  }
+  /* This clever loop by Trent Piepho will take no more than
+   * 8 tries to find a valid direction. */
+  for (n=8; n>0; n--) {
+    int choice = myrand(n);
+    *x = x0 + DIR_DX[dirs[choice]];
+    *y = y0 + DIR_DY[dirs[choice]];
+
+    if (normalize_map_pos(x, y)) /* this neighbour's OK */
+      return;
 
-  *x = x0 + DIR_DX[choice];
-  *y = y0 + DIR_DY[choice];
+    /* Choice was bad, so replace it with the last direction in the list.
+     * On the next iteration, one fewer choices will remain. */
+    dirs[choice] = dirs[n-1];
+  }
 
-  assert(is_real_tile(*x, *y));
-  normalize_map_pos(x, y);
+  assert(0); /* Are we on a 1x1 map with no wrapping??? */
 }
 
 /**************************************************************************
@@ -1340,9 +1345,17 @@
 **************************************************************************/
 const char *dir_get_name(enum direction8 dir)
 {
-  static const char *names[8] = { "NW", "N", "NE", "W",
-    "E", "SW", "S", "SE"
-  };
-  assert(dir >= 0 && dir < 8);
-  return names[dir];
+  /* a switch statement is used so the ordering can be changed easily */
+  switch (dir) {
+    case DIR8_NORTH:     return "N";
+    case DIR8_NORTHEAST: return "NE";
+    case DIR8_EAST:      return "E";
+    case DIR8_SOUTHEAST: return "SE";
+    case DIR8_SOUTH:     return "S";
+    case DIR8_SOUTHWEST: return "SW";
+    case DIR8_WEST:      return "W";
+    case DIR8_NORTHWEST: return "NW";
+  }
+  assert(0);
+  return "[Bad Direction]";
 }