[Freeciv-Dev] Style question

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

While I reviewed the natural city names patch I came across this:

+  /* deal with rivers */
+  if(map_get_special(x, y) & S_RIVER) {
+
+    if(is_terrain_near_tile(x, y, T_OCEAN)) {
+      /* coastal river */
+      for(nptr=nation->default_crcity_names; *nptr; nptr++){
+       if(!game_find_city_by_name(*nptr))
+         return *nptr;
+      }
+    } else {
+      /* non-coastal river */
+      for(nptr=nation->default_rcity_names; *nptr; nptr++){
+       if(!game_find_city_by_name(*nptr))
+         return *nptr;
+      }
+    }
+  }
+
+  /* coastal */
+  if(is_terrain_near_tile(x, y, T_OCEAN)){
+    for(nptr=nation->default_ccity_names; *nptr; nptr++){
+      if(!game_find_city_by_name(*nptr))
+       return *nptr;
+    }
+  }
+
+  /* check terrain type */
+  for(nptr=nation->default_tcity_names[map_get_terrain(x, y)]; *nptr; nptr++){
     if(!game_find_city_by_name(*nptr))
       return *nptr;
   }

+  /* we haven't found a name: it's a normal tile or they're all used */
+  for(nptr=nation->default_city_names; *nptr; nptr++) {
+    if(!game_find_city_by_name(*nptr))
+      return *nptr;
+  }

I think that this is unnecessary code duplication. So was thinking about a

#define SEARCH_AND_RETURN_CITY_NAME(list)   \
    for(nptr=list; *nptr; nptr++) {         \
      if(!game_find_city_by_name(*nptr)) {  \
        return *nptr;                       \
      }                                     \
    }

Which would yield:

  /* deal with rivers */
  if(map_get_special(x, y) & S_RIVER) {
    if(is_terrain_near_tile(x, y, T_OCEAN)) {
      /* coastal river */
      SEARCH_AND_RETURN_CITY_NAME(nation->default_crcity_names)
    } else {
      /* non-coastal river */
      SEARCH_AND_RETURN_CITY_NAME(nation->default_rcity_names)
   }
 }

 /* coastal */
 if(is_terrain_near_tile(x, y, T_OCEAN)){
   SEARCH_AND_RETURN_CITY_NAME(nation->default_ccity_names)
 }

 /* check terrain type */
 SEARCH_AND_RETURN_CITY_NAME(nation->default_tcity_names[map_get_terrain(x, 
y)]);

 /* we haven't found a name: it's a normal tile or they're all used */
 SEARCH_AND_RETURN_CITY_NAME(nation->default_city_names);

Is this a good idea? Comments?

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "There are three ways to get something done. Do it yourself, hire someone
  to do it for you or forbid your kids to do it."