[Freeciv-Dev] Re: Style question

[Daniel Sjölie <deepone@xxxxxxxxxx>]

On 2001-11-30 13:02:14, Raimar Falke wrote:
> 
> 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?

I think it's a good idea...
A function might be better though and an inline function would
probably be best...  But I think any of those are an improvement
so just go for whatever you prefer...

/Daniel

-- 
Now take a deep breath, smile and don't take life so seriously... :)