[Freeciv-Dev] Re: new natural names patch (PR#1127)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sat, Dec 22, 2001 at 12:31:31PM -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
> diff -u -r1.7 nation.h
> --- common/nation.h   2001/12/06 11:59:03     1.7
> +++ common/nation.h   2001/12/22 20:22:14
> @@ -27,6 +27,28 @@
>  enum advisor_type {ADV_ISLAND, ADV_MILITARY, ADV_TRADE, ADV_SCIENCE, 
> ADV_FOREIGN, 
>                     ADV_ATTITUDE, ADV_DOMESTIC, ADV_LAST};
>  
> +/*
> + * The city_name structure holds information about a default choice for
> + * the city name.  The "name" field is, of course, just the name for
> + * the city.  The "value" is the priority rating of this name - lower
> + * priority cities will show up sooner.  The "river" and "terrain" fields
> + * are entries recording whether the terrain is present near the city -
> + * we give higher priority to cities which have matching terrain.  In the
> + * case of a river we only care if the city is _on_ the river, for other
> + * terrain features we give the bonus if the city is close to the
> + * terrain.  Both of these entries may hold a value of 0 (no preference),
> + * 1 (city likes the terrain), or -1 (city doesn't like the terrain).

Either make the three values fix via enum or DEFINE or allow a wider
range (2 stands for really like it and 10 for really really like it).

> +/****************************************************************
> +Searches through a city name list (a struct city_name array)
> +to pick the best available city name, and returns a pointer to
> +it.  The function uses its own internal algorithm to prioritize
> +the city names; this algorithm need not always return the same
> +name when given the same list.  If the list has no valid
> +entries in it, NULL will be returned.
> +*****************************************************************/
> +static char *search_for_city_name(int x, int y, struct city_name *city_names)
> +{
> +  struct city_name *choice;
> +  int best_value = -1, goodness;
> +  char* best_name = NULL;
> +  for (choice = city_names; choice->name; choice++) {
> +    if (!game_find_city_by_name(choice->name)) {
> +      /* Lower values are better. */
> +      int value = choice->value, type;
> +
> +      /* We do a little randomizing of our own. */
> +      value *= 10 + myrand(5);

Why 10? Why 5? E.g. what range does value have?

>  /**************************************************************************
> +  This strips leading and trailing whitespace from a string.
> +
> +  Surely this capability is provided by some other source???

You were lazy. Weren't you. common/shared.c:remove_leading_trailing_spaces

> +/**************************************************************************
> +  This function loads a city name list from a section file.  The file and
> +  two section names (which will be concatenated) are passed in.  The
> +  malloc'ed city name list (which is all filled out) will be returned.
> +**************************************************************************/
> +static struct city_name* load_city_name_list(struct section_file *file,
> +                                          char *secfile_str1,
> +                                          char *secfile_str2)
> +{
> +  char **cities;
> +  int dim, j;
> +  struct city_name *city_names;
> +  int value;
> +
> +  /* First we read the strings from the section file. */
> +  cities = secfile_lookup_str_vec(file, &dim, "%s%s",
> +                               secfile_str1, secfile_str2);
> +
> +  /*
> +   * Now we allocate enough room in the city_names array to store
> +   * all the name data.  The array is NULL-terminated by
> +   * having a NULL name at the end.
> +   */
> +  city_names = fc_calloc(dim + 1, sizeof(struct city_name));
> +  city_names[dim].name = NULL;
> +
> +  /*
> +   * Each string will be of the form
> +   * "<cityname> (<label>, <label>, ...)".  The cityname is just the
> +   * name for this city, while each "label" matches a terrain type
> +   * for the city (or "river"), with a preceeding ! to negate it.  The
                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^

This need updating

> +   * parentheses are optional (but necessary to have the settings, of
> +   * course).  Our job is now to parse this into the city_name structure.
> +   */
> +  for (j = 0, value=10; j < dim; j++, value++) {

Why 10?

> +    /* TODO: handle this more succinctly. */
> +    char *name = cities[j], *next;
> +
> +    /*
> +     * We assign a "base value" to each city based upon its
> +     * position in the list.  We arbitrarily start at 10 and
> +     * count up - higher values mean lower priority, meaning
> +     * the name is less likely to be picked later.
> +     */
> +    city_names[j].value = value;
> +
> +    /*
> +     * Now we wish to determine values for all of the city labels.
> +     * A value of 0 means no preference (which is necessary so that
> +     * the use of this is optional); -1 means the label is negated
> +     * and 1 means it's labelled.  Mostly the parsing just involves
> +     * a lot of ugly string handling...
> +     */

> +    memset(&city_names[j].terrain[0], 0,
> +        T_COUNT * sizeof(city_names[j].terrain[0]));

If somewhere is a
  #define CITY_NAME_DOESNT_CARE 0
is used you can't use the memset.

> +     if (next)
> +       *next = 0;

Extra {} here and in some other places.

> +     strip_space(name);
> +     for (i=0; name[i]; i++)
> +       name[i] = tolower(name[i]);
> +     
> +     /*
> +      * The ! is used to mark a negative, which is recorded
> +      * with a -1.  Otherwise we use a 1.  '-' and '~' have
> +      * the same meaning.
> +      */
> +     if (*name == '!' || *name == '-' || *name == '~') {
> +       name++;
> +       setting = -1;
> +     }
> +     

I would prefer that the "setting = 1" is above the "if" or in an
"else".

> +     /*
> +      * We used to have "coastal" as a special case along
> +      * with river, but this is unnecessary since we
> +      * also have the "ocean" terrain type.
> +      */

Remove this.

> +     if (!strcmp(name, "river")) {
> +       city_names[j].river = setting;
> +     } else {
> +       /* "handled" tracks whether we find a match (for error handling) */
> +       int handled = 0;
> +       enum tile_terrain_type type;
> +     
> +       for (type = T_FIRST; type < T_COUNT && !handled; type++) {
> +         int k;
> +         char namebuf[MAX_LEN_NAME];
> +            /*
> +             * Note that at this time (before a call to
> +             * translate_data_names) the terrain_name fields contains an
> +             * untranslated string. Note that name of T_RIVER is "". However
> +             * this is not a problem because we take care of rivers
> +             * separately.
> +             */

> +            mystrlcpy(namebuf, tile_types[type].terrain_name,
> +             sizeof(tile_types[type].terrain_name));
> +             
> +         /* transform to lower case */
> +         for (k = 0; k < strlen(namebuf); k++) {
> +           namebuf[k] = tolower(namebuf[k]);
> +         }

Doing this transformation for every city in every nation seems
sub-optimal. Although I don't suspect a performance difference the
reader wonders why this is done every time.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  Tank: So what do you need? Besides a miracle.
  Neo: Guns. Lots of guns.
    -- From The Matrix