[Freeciv-Dev] Re: (PR#4044) Revised multiple veteran patch

["Per I. Mathisen" <per@xxxxxxxxxxx>]

On Tue, 22 Apr 2003, James wrote:
> Okay...atlast! :-) Here's the patch for the CVS code. This should apply
> correctly.
> I've only been able to test the gtk client though.

The code looks quite good now. I couldn't trigger any autogame asserts,
either. A few comments, though:

./client/gui-win32/mapctrl.c
+      if (strlen(game.rgame.veteran_name_short[punit->veteran]) == 0) {
+       mystrlcpy(rankstring, "", 1);
+      else {

Missing }

./common/map.c
-  return tile_types[map_get_terrain(x, y)].irrigation_time;
+  int time = tile_types[map_get_terrain(x, y)].irrigation_time;
+  return time * 10;

Lots of these. Just do 'return tile_types[map_get_terrain(x,
y)].irrigation_time * 10'. No need to create an int here.

./data/default/units.ruleset
+veteran_names = "green", "adept", "experienced", "hardened", "veteran",
"elite"

I think we should go down to four levels, as previously suggested on the
list.

+veteran_move_bonus = 0, 0, 1, 1, 2, 3

No units have this flag. Hence all these values should be zero to avoid
confusion.

./server/diplomats.c
+      if (diplomat_success_vs_defender (pdiplomat, punit)) {
                                       ^
Drop the space between function name and its arguments.

+    notify_player_ex(pplayer, x, y, E_MY_DIPLOMAT_ESCAPE,
+                      _("Game: Your %s has successfully completed"
+                        " her mission and returned unharmed to %s%s"),
+                      unit_name(pdiplomat->type), spyhome->name,
+                      vet ? " and has become more veteran." : ".");

"has become more experienced" I'd rather say rather than "has become more
veteran". Please search&replace everywhere if you make the change.

./server/ruleset.c
+    freelog(LOG_FATAL, "In the [veteran_system] section of the %s file,\n"
+              "veteran_names has %i values, but requires that between 1 "
+              "and %i be specified.", filename, num_veteran_levels_found,
+              MAX_VET_LEVELS);
+    exit(EXIT_FAILURE);

Use die() here instead. It combines the two functions above. Same for all
similar cases in this file.

+  if (vblist) free(vblist);

This should be

  if (vblist) {
    free(vblist);
  }

./server/unittools.c
+  /* decrease veteran level by 1 */
+  if (punit->veteran>1) punit->veteran--;

if (punit->veteran > 1) {
  punit->veteran--;
}

Etc.

Finally, there is the question of displaying veteran status. I hope
Raimar's mapdecorations patch can solve this. I don't think we need to
wait for it, though. It can be fixed afterwards.

  - Per