[Freeciv-Dev] Re: new_natural_city_names updated again (PR#1127)

[<per@xxxxxxxxxxx>]

On Tue, 9 Apr 2002, Jason Short wrote:
> This is another update of the new natural city names patch to current
> CVS.  Nothing has really changed; see all the old discussion for an
> explanation.

+struct city_name {
+       char* name;
+       int river;
+       int terrain[T_COUNT];
+};

Can you make it char name[MAX_LEN_NAME] instead? Fewer allocations
better.

-  char **default_city_names;
-  char **default_rcity_names;          /* river city names */
-  char **default_crcity_names;         /* coastal-river city names */
-  char **default_ccity_names;          /* coastal city names */
-  char **default_tcity_names[T_COUNT]; /* terrain-specific city names */
+  struct city_name *city_names;                /* The default city names. */

Thank you. I hated those.

+  if (name)
+    return name;

if (name != NULL) {
  return name;
}

+  if (name)
+    return name;

Ditto.

+      } while (name && *name);

I hate to say it, because I like this a lot myself, but this should be

  while (name != NULL && strlen(name) > 0)

+  GEN_BOOL("naturalcitynames", game.natural_city_names,
+           SSET_RULES_FLEXIBLE, SSET_TO_CLIENT,
+           N_("Whether to use natural city names"),
+           N_("If enabled, the default city names will be determined based "
+              "on the surrounding terrain."),
+           NULL, GAME_DEFAULT_NATURALCITYNAMES)
+

IMHO, you can just make this the default. Why wouldn't anyone want this?
(And even if they didn't want it, would they bother enough to turn it
off? I doubt it.) I don't like unnecessary "options creep".

Overall, I think the patch looks good and is a definite improvement. Not
tested.

Yours,
Per

"While we're all in favor of Arafat doing more to suppress violence in
the Middle East, we fear that he doesn't have all that much influence on
the Israeli government. " --Sam Smith