widelands-dev team mailing list archive

[Hans Joachim Desserud <launchpad@xxxxxxxxxxxx>]

Hans Joachim Desserud has proposed merging lp:~hjd/widelands/codecheck-and-deduplication into lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~hjd/widelands/codecheck-and-deduplication/+merge/257464

I started looking at a whitespace codecheck issue, but noticed some duplicated code nearby and ended up fixing that as well.

The first section in each case of this if was identical, so I restructured it to place the conditional check inside the loop and always run the common code before checking sparam1 more thoroughly.

Could arguably be refactored further into its own method if that makes the code more readable.
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~hjd/widelands/codecheck-and-deduplication into lp:widelands.

=== modified file 'src/logic/worker.cc'
--- src/logic/worker.cc	2015-04-22 19:13:03 +0000
+++ src/logic/worker.cc	2015-04-25 17:39:47 +0000
@@ -396,22 +396,21 @@
 	Area<FCoords> area (map.get_fcoords(get_position()), 0);
 	bool found_reserved = false;
 
-	if (action.sparam1 == "immovable") {
-
-		for (;; ++area.radius) {
-			if (action.iparam1 < area.radius) {
-				send_signal(game, "fail"); //  no object found, cannot run program
-				pop_task(game);
-				if (upcast(ProductionSite, productionsite, get_location(game))) {
-					if (!found_reserved) {
-						productionsite->notify_player(game, 30);
-					}
-					else {
-						productionsite->unnotify_player();
-					}
-				}
-				return true;
+	for (;; ++area.radius) {
+		if (action.iparam1 < area.radius) {
+			send_signal(game, "fail"); //  no object found, cannot run program
+			pop_task(game);
+			if (upcast(ProductionSite, productionsite, get_location(game))) {
+				if (!found_reserved) {
+					productionsite->notify_player(game, 30);
+				}
+				else {
+					productionsite->unnotify_player();
+				}
 			}
+			return true;
+		}
+		if (action.sparam1 == "immovable") {
 			std::vector<ImmovableFound> list;
 			if (action.iparam2 < 0)
 				map.find_reachable_immovables
@@ -443,25 +442,9 @@
 						(game, state, list[game.logic_rand() % list.size()].object);
 				break;
 			}
-		}
-	} else {
-		for (;; ++area.radius) {
-			if (action.iparam1 < area.radius) {
-				send_signal(game, "fail"); //  no object found, cannot run program
-				pop_task(game);
-				if (upcast(ProductionSite, productionsite, get_location(game))){
-					if (!found_reserved) {
-						productionsite->notify_player(game, 30);
-					}
-					else {
-						productionsite->unnotify_player();
-					}
-				}
-				return true;
-			}
-			else {
-			  if ( upcast(ProductionSite, productionsite, get_location(game)))
-				        productionsite->unnotify_player();
+		} else {
+			if (upcast(ProductionSite, productionsite, get_location(game))) {
+				productionsite->unnotify_player();
 			}
 			std::vector<Bob *> list;
 			if (action.iparam2 < 0)