[Freeciv-Dev] Re: How to make client code 22 lines shorter. (PR#1240)

[jdorje@xxxxxxxxxxxxxxxxxxxxx]

Gregory Berkolaiko wrote:

> There is triplicated code in create_goto_map in client/goto.c
> The lines 370-380, 404-414 and 429-439 are identical and can be therefore
> put outside the switch statement.
>
> Jason, you did exactly the same thing in gotohand.c once (only there it
> was a bit more complicated), do you want to do it here too?
Seems simple enough...

It also seems possible to move the quick-check on the potential goodness 
of the route up above the switch statement.
The correctness of the patch should be fairly obvious:

- The code replaced is (AFAICT) entirely identical.
- There are no extra or missing breaks within the switch statement.
- All variables remain within scope.
- Logically, it makes sense that the same algorithm is being used.

The trick, of course, would be to merge this with the server code 
(moving it into common; they are, in theory, identical algorithms). 
Unfortunately they use different backend structures (warmap versus 
goto_map), so this is a larger task.
jason

Index: client/goto.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/goto.c,v
retrieving revision 1.27
diff -u -r1.27 goto.c
--- client/goto.c       2001/10/08 12:11:16     1.27
+++ client/goto.c       2002/01/24 11:37:13
@@ -320,12 +320,14 @@
       pdesttile = map_get_tile(x1, y1);
       add_to_queue = 1;
 
+      if (goto_map.move_cost[x1][y1] <= goto_map.move_cost[x][y]) {
+       /* No need for all the calculations. Note that this also excludes
+        * RR loops, ie you can't create a cycle with the same move_cost */
+       continue;
+      }
+
       switch (move_type) {
       case LAND_MOVING:
-       if (goto_map.move_cost[x1][y1] <= goto_map.move_cost[x][y])
-         continue; /* No need for all the calculations. Note that this also 
excludes
-                      RR loops, ie you can't create a cycle with the same 
move_cost */
-
        if (pdesttile->terrain == T_OCEAN) {
          if (ground_unit_transporter_capacity(x1, y1, unit_owner(punit))
              <= 0)
@@ -366,24 +368,10 @@
          }
        } else if (!goto_zoc_ok(punit, x, y, x1, y1))
          continue;
-
-       /* Add the route to our warmap if it is worth keeping */
-       total_cost = move_cost + goto_map.move_cost[x][y];
-       if (goto_map.move_cost[x1][y1] > total_cost) {
-         goto_map.move_cost[x1][y1] = total_cost;
-         if (add_to_queue)
-           add_to_mapqueue(total_cost, x1, y1);
-         goto_map.vector[x1][y1] = 1 << DIR_REVERSE(dir);
-         freelog(LOG_DEBUG,
-                 "Candidate: %s from (%d, %d) to (%d, %d), cost %d",
-                 dir_get_name(dir), x, y, x1, y1, total_cost);
-       }
+       
        break;
 
       case SEA_MOVING:
-       if (goto_map.move_cost[x1][y1] <= goto_map.move_cost[x][y])
-         continue; /* No need for all the calculations */
-
        if (pdesttile->terrain == T_UNKNOWN) {
          move_cost = 2*SINGLE_MOVE; /* arbitrary */
        } else if (is_non_allied_unit_tile(pdesttile, unit_owner(punit))
@@ -399,25 +387,10 @@
          move_cost = SINGLE_MOVE;
        }
 
-       total_cost = move_cost + goto_map.move_cost[x][y];
-
-       /* Add the route to our warmap if it is worth keeping */
-       if (goto_map.move_cost[x1][y1] > total_cost) {
-         goto_map.move_cost[x1][y1] = total_cost;
-         if (add_to_queue)
-           add_to_mapqueue(total_cost, x1, y1);
-         goto_map.vector[x1][y1] = 1 << DIR_REVERSE(dir);
-         freelog(LOG_DEBUG,
-                 "Candidate: %s from (%d, %d) to (%d, %d), cost %d",
-                 dir_get_name(dir), x, y, x1, y1, total_cost);
-       }
        break;
 
       case AIR_MOVING:
       case HELI_MOVING:
-       if (goto_map.move_cost[x1][y1] <= goto_map.move_cost[x][y])
-         continue; /* No need for all the calculations */
-
        move_cost = SINGLE_MOVE;
        /* Planes could run out of fuel, therefore we don't care if territory
           is unknown. Also, don't attack except at the destination. */
@@ -425,18 +398,6 @@
        if (is_non_allied_unit_tile(pdesttile, unit_owner(punit))) {
          add_to_queue = 0;
        }
-
-       /* Add the route to our warmap if it is worth keeping */
-       total_cost = move_cost + goto_map.move_cost[x][y];
-       if (goto_map.move_cost[x1][y1] > total_cost) {
-         goto_map.move_cost[x1][y1] = total_cost;
-         if (add_to_queue)
-           add_to_mapqueue(total_cost, x1, y1);
-         goto_map.vector[x1][y1] = 1 << DIR_REVERSE(dir);
-         freelog(LOG_DEBUG,
-                 "Candidate: %s from (%d, %d) to (%d, %d), cost %d",
-                 dir_get_name(dir), x, y, x1, y1, total_cost);
-       }
        break;
 
       default:
@@ -445,6 +406,20 @@
        abort();
        break;
       } /****** end switch ******/
+
+      /* Add the route to our warmap if it is worth keeping */
+      total_cost = move_cost + goto_map.move_cost[x][y];
+      if (goto_map.move_cost[x1][y1] > total_cost) {
+       goto_map.move_cost[x1][y1] = total_cost;
+       if (add_to_queue) {
+         add_to_mapqueue(total_cost, x1, y1);
+       }
+       goto_map.vector[x1][y1] = 1 << DIR_REVERSE(dir);
+       freelog(LOG_DEBUG,
+               "Candidate: %s from (%d, %d) to (%d, %d), cost %d",
+               dir_get_name(dir), x, y, x1, y1, total_cost);
+      }
+
     } adjc_dir_iterate_end;
   } /* end while */
 }