[Freeciv-Dev] Re: Pop cost patch (resending via bug system) (PR#897)

[Arien Malec <arien_malec@xxxxxxxxx>]

OK -- attached is a patch that cleans up handle_unit_build_city considerably,
but may move ugliness to other places:

1) can_unit_add_to_city and can_unit_build_city are gone, replaced by
can_unit_add_or_build_city
2) can_unit_add_or_build_city centralizes all the checking and logic for city
adding/building, and centralizes all the consistency checking
3) handle_unit_build_city is broken into three cases: adding, building, and
error, each handled by static functions

Issues:
The static functions trust that can_unit_or_build_city does indeed do all the
consistency checking. For instance, if can_unit_add_or_build_city returns
AB_NO_AQUEDUCT, city_add_build_error doesn't bother to check that pcity !=
NULL. It may be appropriate to sprinkle some assertions about???

the_names_of_things_are_long

Arien

__________________________________________________
Do You Yahoo!?
Make international calls for as low as $.04/minute with Yahoo! Messenger
http://phonecard.yahoo.com/

Index: client/control.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/control.c,v
retrieving revision 1.58
diff -u -r1.58 control.c
--- client/control.c    2001/06/29 19:39:00     1.58
+++ client/control.c    2001/08/22 21:51:20
@@ -567,7 +567,7 @@
 **************************************************************************/
 void request_unit_build_city(struct unit *punit)
 {
-  if(can_unit_build_city(punit)) {
+  if(can_unit_add_or_build_city(punit) == AB_BUILD_OK) {
     struct packet_generic_integer req;
     req.value = punit->id;
     send_packet_generic_integer(&aconnection,
Index: client/gui-gtk/menu.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-gtk/menu.c,v
retrieving revision 1.55
diff -u -r1.55 menu.c
--- client/gui-gtk/menu.c       2001/07/02 19:51:20     1.55
+++ client/gui-gtk/menu.c       2001/08/22 21:51:21
@@ -304,9 +304,8 @@
       struct unit *punit = get_unit_in_focus();
       /* Enable the button for adding to a city in all cases, so we
         get an eventual error message from the server if we try. */
-      if (can_unit_build_city(punit)
-         || (unit_flag(punit->type, F_CITIES)
-             && map_get_city(punit->x, punit->y))) {
+      if (can_unit_add_or_build_city(punit) == AB_BUILD_OK ||
+         can_unit_add_or_build_city(punit) == AB_ADD_OK) {
        key_unit_build_city();
       } else {
        key_unit_build_wonder();
@@ -1000,9 +999,8 @@
       /* Enable the button for adding to a city in all cases, so we
         get an eventual error message from the server if we try. */
       menus_set_sensitive("<main>/_Orders/_Build City",
-                         (can_unit_build_city(punit) ||
-                           (unit_flag(punit->type, F_CITIES)
-                           && map_get_city(punit->x, punit->y)) ||
+                         (can_unit_add_or_build_city(punit) == AB_BUILD_OK ||
+                           can_unit_add_or_build_city(punit) == AB_ADD_OK ||
                            unit_can_help_build_wonder_here(punit)));
       menus_set_sensitive("<main>/_Orders/Build _Road",
                           (can_unit_do_activity(punit, ACTIVITY_ROAD) ||
Index: client/gui-mui/gui_main.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-mui/gui_main.c,v
retrieving revision 1.54
diff -u -r1.54 gui_main.c
--- client/gui-mui/gui_main.c   2001/08/18 20:33:06     1.54
+++ client/gui-mui/gui_main.c   2001/08/22 21:51:22
@@ -1270,7 +1270,9 @@
 
       set(main_menu, MUIA_Window_Menustrip, NULL);
 
-      menu_entry_sensitive(MENU_ORDER_BUILD_CITY, (can_unit_build_city(punit) 
|| can_unit_add_to_city(punit)));
+      menu_entry_sensitive(MENU_ORDER_BUILD_CITY,
+                          (can_unit_add_or_build_city(punit) == AB_BUILD_OK ||
+                           can_unit_add_or_build_city(punit) == AB_ADD_OK));
       menu_entry_sensitive(MENU_ORDER_ROAD, can_unit_do_activity(punit, 
ACTIVITY_ROAD) || can_unit_do_activity(punit, ACTIVITY_RAILROAD));
       menu_entry_sensitive(MENU_ORDER_IRRIGATE, can_unit_do_activity(punit, 
ACTIVITY_IRRIGATE));
       menu_entry_sensitive(MENU_ORDER_MINE, can_unit_do_activity(punit, 
ACTIVITY_MINE));
Index: client/gui-mui/mapclass.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-mui/mapclass.c,v
retrieving revision 1.59
diff -u -r1.59 mapclass.c
--- client/gui-mui/mapclass.c   2001/08/22 07:53:25     1.59
+++ client/gui-mui/mapclass.c   2001/08/22 21:51:24
@@ -1817,9 +1817,9 @@
              if ((list.pool = CreatePool(0, 1024, 1024)))
              {
                /* Note: Must be better done (linked with the pull down menu */
-               if (can_unit_build_city(punit))
+               if (can_unit_add_or_build_city(punit) == AB_BUILD_OK)
                  Map_InsertCommand(&list, _("Build City"), 
PACK_USERDATA(punit, MENU_ORDER_BUILD_CITY));
-               if (can_unit_add_to_city(punit))
+               if (can_unit_add_or_build_city(punit) == AB_ADD_OK)
                  Map_InsertCommand(&list, _("Add to City"), 
PACK_USERDATA(punit, MENU_ORDER_BUILD_CITY));
                if (can_unit_do_activity(punit, ACTIVITY_ROAD))
                  Map_InsertCommand(&list, _("Build Road"), 
PACK_USERDATA(punit, MENU_ORDER_ROAD));
Index: client/gui-xaw/menu.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-xaw/menu.c,v
retrieving revision 1.45
diff -u -r1.45 menu.c
--- client/gui-xaw/menu.c       2001/06/29 19:39:04     1.45
+++ client/gui-xaw/menu.c       2001/08/22 21:51:24
@@ -309,8 +309,8 @@
       tinfo = get_tile_type(ttype);
 
       menu_entry_sensitive(MENU_ORDER, MENU_ORDER_BUILD_CITY, 
-                          (can_unit_build_city(punit) ||
-                           can_unit_add_to_city(punit)));
+                          (can_unit_add_or_build_city(punit) == AB_BUILD_OK ||
+                           can_unit_add_or_build_add_city(punit) == 
AB_ADD_OK));
       menu_entry_sensitive(MENU_ORDER, MENU_ORDER_ROAD, 
                           can_unit_do_activity(punit, ACTIVITY_ROAD) ||
                           can_unit_do_activity(punit, ACTIVITY_RAILROAD));
Index: common/unit.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/unit.c,v
retrieving revision 1.125
diff -u -r1.125 unit.c
--- common/unit.c       2001/08/13 12:25:26     1.125
+++ common/unit.c       2001/08/22 21:51:25
@@ -388,21 +388,6 @@
   return 0;
 }
 
-
-/**************************************************************************
-...
-**************************************************************************/
-int can_unit_build_city(struct unit *punit)
-{
-  if(!unit_flag(punit->type, F_CITIES))
-    return 0;
-
-  if(!punit->moves_left)
-    return 0;
-
-  return city_can_be_built_here(punit->x, punit->y);
-}
-
 /**************************************************************************
 ...
 **************************************************************************/
@@ -413,35 +398,42 @@
 /**************************************************************************
 ...
 **************************************************************************/
-int can_unit_add_to_city(struct unit *punit)
+enum add_build_city_result
+can_unit_add_or_build_city (struct unit *punit)
 {
   struct city *pcity;
-
-  if(!unit_flag(punit->type, F_CITIES))
-    return 0;
-  if(!punit->moves_left)
-    return 0;
-
   pcity = map_get_city(punit->x, punit->y);
 
-  if(!pcity)
-    return 0;
+  if(!unit_flag(punit->type, F_CITIES))
+    return AB_NOT_ADDABLE_UNIT;
+  if(!punit->moves_left) {
+    if (!pcity)
+      return AB_NO_MOVES_BUILD;
+    else
+      return AB_NO_MOVES_ADD;
+  }
+  if(!pcity) {
+    if (city_can_be_built_here (punit->x, punit->y))
+      return AB_BUILD_OK;
+    else
+      return AB_NOT_BUILD_LOC;
+  }
   if(pcity->size >= game.add_to_size_limit)
-    return 0;
+    return AB_TOO_BIG;
   if(pcity->owner != punit->owner)
-    return 0;
+    return AB_NOT_OWNER;
 
   if(improvement_exists(B_AQUEDUCT)
      && !city_got_building(pcity, B_AQUEDUCT) 
      && pcity->size >= game.aqueduct_size)
-    return 0;
+    return AB_NO_AQUEDUCT;
   
   if(improvement_exists(B_SEWER)
      && !city_got_building(pcity, B_SEWER)
      && pcity->size >= game.sewer_size)
-    return 0;
+    return AB_NO_SEWER;
 
-  return 1;
+  return AB_ADD_OK;
 }
 
 /**************************************************************************
Index: common/unit.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/unit.h,v
retrieving revision 1.72
diff -u -r1.72 unit.h
--- common/unit.h       2001/08/13 12:25:27     1.72
+++ common/unit.h       2001/08/22 21:51:25
@@ -66,6 +66,24 @@
   MR_OTHER, MR_INVALID_TYPE_FOR_CITY_TAKE_OVER, MR_NO_WAR, MR_ZOC
 };
 
+enum add_build_city_result {
+  AB_BUILD_OK,         /* Unit OK to build city */
+  AB_ADD_OK,           /* Unit OK to add to city */
+  AB_NOT_BUILD_LOC,    /* City is not allowed to be built at this
+                         location */
+  AB_NOT_ADDABLE_UNIT, /* Unit is not one that can be added to cities */
+  AB_NO_MOVES_BUILD,   /* Unit does not have moves left to build a
+                         city */
+  AB_NO_MOVES_ADD,     /* Unit does not have moves left to add to
+                         city */
+  AB_NOT_OWNER,        /* Owner of unit is not owner of city */
+  AB_TOO_BIG,          /* City is too big to be added to */
+  AB_NO_AQUEDUCT,      /* Adding takes city past limit for aquaduct
+                         but city has no aquaduct */
+  AB_NO_SEWER          /* Adding takes city past limit for sewer but
+                         city has no sewer */
+};
+
 struct unit_ai {
   int control; /* 0: not automated    1: automated */
   enum ai_unit_task ai_role;
@@ -185,8 +203,7 @@
 int is_air_unit(struct unit *punit);
 int is_heli_unit(struct unit *punit);
 int is_ground_unit(struct unit *punit);
-int can_unit_build_city(struct unit *punit);
-int can_unit_add_to_city(struct unit *punit);
+enum add_build_city_result can_unit_add_or_build_city (struct unit *punit);
 int kills_citizen_after_attack(struct unit *punit);
 
 char *unit_activity_text(struct unit *punit);
Index: data/default/cities.ruleset
===================================================================
RCS file: /home/freeciv/CVS/freeciv/data/default/cities.ruleset,v
retrieving revision 1.6
diff -u -r1.6 cities.ruleset
--- data/default/cities.ruleset 2001/05/24 22:12:09     1.6
+++ data/default/cities.ruleset 2001/08/22 21:51:25
@@ -12,7 +12,7 @@
 options="1.9"
 
 [parameters]
-add_to_size_limit  = 8         ; cities >= this cannot be added to.
+add_to_size_limit  = 18                ; cities >= this cannot be added to.
 
 ;
 ; City styles define the way cities are drawn
Index: server/unithand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unithand.c,v
retrieving revision 1.193
diff -u -r1.193 unithand.c
--- server/unithand.c   2001/08/08 09:46:26     1.193
+++ server/unithand.c   2001/08/22 21:51:26
@@ -52,6 +52,12 @@
 
 #include "unithand.h"
 
+static void city_add_or_build_error(struct player *pplayer,
+                                   struct unit *punit,
+                                   enum add_build_city_result res);
+static void city_add_unit (struct player *pplayer, struct unit *punit);
+static void city_build (struct player *pplayer, struct unit *punit, char 
*name);
+
 /**************************************************************************
 ...
 **************************************************************************/
@@ -404,103 +410,137 @@
 /**************************************************************************
 ...
 **************************************************************************/
-void handle_unit_build_city(struct player *pplayer, 
-                           struct packet_unit_request *req)
+static void city_add_or_build_error(struct player *pplayer,
+                                   struct unit *punit,
+                                   enum add_build_city_result res)
 {
-  struct unit *punit;
-  char *unit_name;
   struct city *pcity;
-  char *name;
-  
-  punit = player_find_unit_by_id(pplayer, req->unit_id) ;
-  if(!punit)
-    return;
+  char *unit_name;
   
   unit_name = get_unit_type(punit->type)->name;
+  /* Given that res came from can_unit_add_or_build_city, pcity
+     will be non-null for all required status values */
   pcity = map_get_city(punit->x, punit->y);
-  
-  if (!unit_flag(punit->type, F_CITIES)) {
-    char *us = get_units_with_flag_string(F_CITIES);
+
+  switch (res) {
+  case AB_NOT_BUILD_LOC:
     notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT,
-                    _("Game: Only %s can build or add to a city."), us);
-    free(us);
+                    _("Game: Can't place city here."));
     return;
-  }  
-
-  if(!punit->moves_left)  {
-    if (pcity) {
-      notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT,
-                      _("Game: %s unit has no moves left to add to %s."),
-                      unit_name, pcity->name);
-    } else {
+  case AB_NOT_ADDABLE_UNIT:
+    {
+      char *us = get_units_with_flag_string(F_CITIES);
       notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT,
-                      _("Game: %s unit has no moves left to build city."),
-                      unit_name);
+                      _("Game: Only %s can build or add to a city."), us);
+      free(us);
+      return;
     }
+  case AB_NO_MOVES_ADD:
+    notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT,
+                    _("Game: %s unit has no moves left to add to %s."),
+                    unit_name, pcity->name);
     return;
-  }
-    
-  if (pcity) {
-    if (can_unit_add_to_city(punit)) {
-      pcity->size++;
-      if (!add_adjust_workers(pcity)) {
-       auto_arrange_workers(pcity);
-       sync_cities();
-      }
-      wipe_unit(punit);
-      send_city_info(0, pcity);
-      notify_player_ex(pplayer, pcity->x, pcity->y, E_NOEVENT, 
-                      _("Game: %s added to aid %s in growing."), 
-                      unit_name, pcity->name);
-    } else {
-      if(pcity->size >= game.add_to_size_limit) {
-       notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT, 
-                        _("Game: %s is too big to add %s."),
-                        pcity->name, unit_name);
-      }
-      else if(improvement_exists(B_AQUEDUCT)
-        && !city_got_building(pcity, B_AQUEDUCT) 
-        && pcity->size >= game.aqueduct_size) {
-       notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT, 
-                        _("Game: %s needs %s to grow, so you cannot add %s."),
-                        pcity->name, get_improvement_name(B_AQUEDUCT),
-                        unit_name);
-      }
-      else if(improvement_exists(B_SEWER)
-             && !city_got_building(pcity, B_SEWER)
-             && pcity->size >= game.sewer_size) {
-       notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT, 
-                        _("Game: %s needs %s to grow, so you cannot add %s."),
-                        pcity->name, get_improvement_name(B_SEWER),
-                        unit_name);
-      }
-      else {
-       /* this shouldn't happen? */
-       freelog(LOG_ERROR, "Cannot add %s to %s for unknown reason",
-               unit_name, pcity->name);
-       notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT, 
-                        _("Game: cannot add %s to %s."),
-                        unit_name, pcity->name);
-      }
-    }
+  case AB_NO_MOVES_BUILD:
+    notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT,
+                    _("Game: %s unit has no moves left to build city."),
+                    unit_name);
+    return;
+  case AB_TOO_BIG:
+    notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT, 
+                    _("Game: %s is too big to add %s."),
+                    pcity->name, unit_name);
     return;
+  case AB_NO_AQUEDUCT:
+    notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT, 
+                    _("Game: %s needs %s to grow, so you cannot add %s."),
+                    pcity->name, get_improvement_name(B_AQUEDUCT),
+                    unit_name);
+    return;
+  case AB_NO_SEWER:
+    notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT, 
+                    _("Game: %s needs %s to grow, so you cannot add %s."),
+                    pcity->name, get_improvement_name(B_SEWER),
+                    unit_name);
+    return;
+  default:
+    /* Shouldn't happen */
+    freelog(LOG_ERROR, "Cannot add %s to %s for unknown reason",
+           unit_name, pcity->name);
+    notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT, 
+                    _("Game: cannot add %s to %s."),
+                    unit_name, pcity->name);
   }
-    
-  if(!can_unit_build_city(punit)) {
-      notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT,
-                      _("Game: Can't place city here."));
-      return;
+  return;
+}
+
+/**************************************************************************
+...
+**************************************************************************/
+static void city_add_unit (struct player *pplayer,
+                          struct unit *punit)
+{
+  struct city *pcity;
+  char *unit_name;
+  pcity = map_get_city(punit->x, punit->y);
+  unit_name = get_unit_type(punit->type)->name;
+
+  pcity->size++;
+  if (!add_adjust_workers(pcity)) {
+    auto_arrange_workers(pcity);
+    sync_cities();
   }
-      
-  if(!(name=get_sane_name(req->name))) {
+  wipe_unit(punit);
+  send_city_info(0, pcity);
+  notify_player_ex(pplayer, pcity->x, pcity->y, E_NOEVENT, 
+                  _("Game: %s added to aid %s in growing."), 
+                  unit_name, pcity->name);
+  return;
+}
+
+/**************************************************************************
+...
+**************************************************************************/
+static void city_build (struct player *pplayer, struct unit *punit, char *name)
+{
+  char *city_name;
+  if(!(city_name=get_sane_name(name))) {
     notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT, 
                     _("Game: Let's not build a city with such a stupid 
name."));
     return;
   }
-
-  create_city(pplayer, punit->x, punit->y, name);
+  create_city(pplayer, punit->x, punit->y, city_name);
   wipe_unit(punit);
+  return;
+}
+
+
+/**************************************************************************
+...
+**************************************************************************/
+void handle_unit_build_city(struct player *pplayer, 
+                           struct packet_unit_request *req)
+{
+  struct unit *punit;
+  enum add_build_city_result res;
+  
+  punit = player_find_unit_by_id(pplayer, req->unit_id) ;
+  if(!punit)
+    return;
+  
+  res = can_unit_add_or_build_city (punit);
+
+  if (res == AB_BUILD_OK)
+    city_build (pplayer, punit, req->name);
+  else if (res == AB_ADD_OK)
+    city_add_unit (pplayer, punit);
+  else
+    city_add_or_build_error (pplayer, punit, res);
+
+  return;
 }
+
+  
+
 
 /**************************************************************************
 ...