Merge pull request #131 from digitall/goto_considered_harmful

Goto Considered Harmful...

The following commits should improve the ScummVM code structure by reducing the number of gotos used in various engine code.

They should implement identical functionality, but without using goto and without the result being less readable/maintainable than the version with goto.

2 files changed, 185 insertions, 173 deletions

diff --git a/engines/lure/hotspots.cpp b/engines/lure/hotspots.cpp
index 207c125a0c..2f6d0f23aa 100644
--- a/engines/lure/hotspots.cpp
+++ b/engines/lure/hotspots.cpp
@@ -4164,6 +4164,7 @@ PathFinderResult PathFinder::process() {
 	bool altFlag;
 	uint16 *pCurrent;
 	PathFinderResult result = PF_UNFINISHED;
+	bool skipToFinalStep = false;
 
 	if (!_inProgress) {
 		// Following code only done during first call to method
@@ -4186,188 +4187,190 @@ PathFinderResult PathFinder::process() {
 
 			_inProgress = false;
 			result = PF_OK;
-			goto final_step;
-		}
-
-		// Path finding
+			skipToFinalStep = true;
+		} else {
+			// Path finding
 
-		_destX >>= 3;
-		_destY >>= 3;
-		_pSrc = &_layer[(_yCurrent + 1) * DECODED_PATHS_WIDTH + 1 + _xCurrent];
-		_pDest = &_layer[(_yDestCurrent + 1) * DECODED_PATHS_WIDTH + 1 + _xDestCurrent];
+			_destX >>= 3;
+			_destY >>= 3;
+			_pSrc = &_layer[(_yCurrent + 1) * DECODED_PATHS_WIDTH + 1 + _xCurrent];
+			_pDest = &_layer[(_yDestCurrent + 1) * DECODED_PATHS_WIDTH + 1 + _xDestCurrent];
 
-		// Flag starting/ending cells
-		*_pSrc = 1;
-		_destOccupied = *_pDest != 0;
-		result = _destOccupied ? PF_DEST_OCCUPIED : PF_OK;
-		*_pDest = 0;
+			// Flag starting/ending cells
+			*_pSrc = 1;
+			_destOccupied = *_pDest != 0;
+			result = _destOccupied ? PF_DEST_OCCUPIED : PF_OK;
+			*_pDest = 0;
 
-		// Set up the current pointer, adjusting away from edges if necessary
+			// Set up the current pointer, adjusting away from edges if necessary
 
-		if (_xCurrent >= _xDestCurrent) {
-			_xChangeInc = -1;
-			_xChangeStart = ROOM_PATHS_WIDTH;
-		} else {
-			_xChangeInc = 1;
-			_xChangeStart = 1;
-		}
+			if (_xCurrent >= _xDestCurrent) {
+				_xChangeInc = -1;
+				_xChangeStart = ROOM_PATHS_WIDTH;
+			} else {
+				_xChangeInc = 1;
+				_xChangeStart = 1;
+			}
 
-		if (_yCurrent >= _yDestCurrent) {
-			_yChangeInc = -1;
-			_yChangeStart = ROOM_PATHS_HEIGHT;
-		} else {
-			_yChangeInc = 1;
-			_yChangeStart = 1;
+			if (_yCurrent >= _yDestCurrent) {
+				_yChangeInc = -1;
+				_yChangeStart = ROOM_PATHS_HEIGHT;
+			} else {
+				_yChangeInc = 1;
+				_yChangeStart = 1;
+			}
 		}
 	}
 
-	// Major loop to populate data
-	_cellPopulated = false;
-
-	while (1) {
-		// Loop through to process cells in the given area
-		if (!returnFlag) _yCtr = 0;
-		while (returnFlag || (_yCtr < ROOM_PATHS_HEIGHT)) {
-			if (!returnFlag) _xCtr = 0;
+	if (!skipToFinalStep) {
+		// Major loop to populate data
+		_cellPopulated = false;
 
-			while (returnFlag || (_xCtr < ROOM_PATHS_WIDTH)) {
-				if (!returnFlag) {
-					processCell(&_layer[(_yChangeStart + _yCtr * _yChangeInc) * DECODED_PATHS_WIDTH +
-						(_xChangeStart + _xCtr * _xChangeInc)]);
-					if (breakFlag && (_countdownCtr <= 0)) return PF_UNFINISHED;
-				} else {
-					returnFlag = false;
+		while (1) {
+			// Loop through to process cells in the given area
+			if (!returnFlag) _yCtr = 0;
+			while (returnFlag || (_yCtr < ROOM_PATHS_HEIGHT)) {
+				if (!returnFlag) _xCtr = 0;
+
+				while (returnFlag || (_xCtr < ROOM_PATHS_WIDTH)) {
+					if (!returnFlag) {
+						processCell(&_layer[(_yChangeStart + _yCtr * _yChangeInc) * DECODED_PATHS_WIDTH +
+							(_xChangeStart + _xCtr * _xChangeInc)]);
+						if (breakFlag && (_countdownCtr <= 0)) return PF_UNFINISHED;
+					} else {
+						returnFlag = false;
+					}
+					++_xCtr;
 				}
-				++_xCtr;
+				++_yCtr;
 			}
-			++_yCtr;
-		}
 
-		// If the destination cell has been filled in, then break out of loop
-		if (*_pDest != 0) break;
+			// If the destination cell has been filled in, then break out of loop
+			if (*_pDest != 0) break;
 
-		if (_cellPopulated) {
-			// At least one cell populated, so go repeat loop
-			_cellPopulated = false;
-		} else {
-			result = PF_PART_PATH;
-			scanFlag = true;
-			break;
+			if (_cellPopulated) {
+				// At least one cell populated, so go repeat loop
+				_cellPopulated = false;
+			} else {
+				result = PF_PART_PATH;
+				scanFlag = true;
+				break;
+			}
 		}
-	}
-	_inProgress = false;
+		_inProgress = false;
 
-	if (scanFlag || _destOccupied) {
-		// Adjust the end point if necessary to stop character walking into occupied area
+		if (scanFlag || _destOccupied) {
+			// Adjust the end point if necessary to stop character walking into occupied area
 
-		// Restore destination's occupied state if necessary
-		if (_destOccupied) {
-			*_pDest = 0xffff;
-			_destOccupied = false;
-		}
+			// Restore destination's occupied state if necessary
+			if (_destOccupied) {
+				*_pDest = 0xffff;
+				_destOccupied = false;
+			}
 
-		// Scan through lines
-		v = 0xff;
-		pTemp = _pDest;
-		scanLine(_destX, -1, pTemp, v);
-		scanLine(ROOM_PATHS_WIDTH - _destX, 1, pTemp, v);
-		scanLine(_destY, -DECODED_PATHS_WIDTH, pTemp, v);
-		scanLine(ROOM_PATHS_HEIGHT - _destY, DECODED_PATHS_WIDTH, pTemp, v);
+			// Scan through lines
+			v = 0xff;
+			pTemp = _pDest;
+			scanLine(_destX, -1, pTemp, v);
+			scanLine(ROOM_PATHS_WIDTH - _destX, 1, pTemp, v);
+			scanLine(_destY, -DECODED_PATHS_WIDTH, pTemp, v);
+			scanLine(ROOM_PATHS_HEIGHT - _destY, DECODED_PATHS_WIDTH, pTemp, v);
+
+			if (pTemp == _pDest) {
+				clear();
+				return PF_NO_WALK;
+			}
 
-		if (pTemp == _pDest) {
-			clear();
-			return PF_NO_WALK;
+			_pDest = pTemp;
 		}
 
-		_pDest = pTemp;
-	}
+		// ****DEBUG****
+		if (_hotspot->hotspotId() == PLAYER_ID) {
+			for (int ctr = 0; ctr < DECODED_PATHS_WIDTH * DECODED_PATHS_HEIGHT; ++ctr)
+				Room::getReference().tempLayer[ctr] = _layer[ctr];
+		}
 
-	// ****DEBUG****
-	if (_hotspot->hotspotId() == PLAYER_ID) {
-		for (int ctr = 0; ctr < DECODED_PATHS_WIDTH * DECODED_PATHS_HEIGHT; ++ctr)
-			Room::getReference().tempLayer[ctr] = _layer[ctr];
-	}
+		// Determine the walk path by working backwards from the destination, adding in the
+		// walking steps in reverse order until source is reached
+		int stageCtr;
+		for (stageCtr = 0; stageCtr < 3; ++stageCtr) {
+			// Clear out any previously determined directions
+			clear();
 
-	// Determine the walk path by working backwards from the destination, adding in the
-	// walking steps in reverse order until source is reached
-	int stageCtr;
-	for (stageCtr = 0; stageCtr < 3; ++stageCtr) {
-		// Clear out any previously determined directions
-		clear();
+			altFlag = stageCtr == 1;
+			pCurrent = _pDest;
+
+			numSteps = 0;
+			currDirection = NO_DIRECTION;
+			while (1) {
+				v = *pCurrent - 1;
+				if (v == 0) break;
+
+				newDirection = NO_DIRECTION;
+				if (!altFlag && (currDirection != LEFT) && (currDirection != RIGHT)) {
+					// Standard order direction checking
+					if (*(pCurrent - DECODED_PATHS_WIDTH) == v) newDirection = DOWN;
+					else if (*(pCurrent + DECODED_PATHS_WIDTH) == v) newDirection = UP;
+					else if (*(pCurrent + 1) == v) newDirection = LEFT;
+					else if (*(pCurrent - 1) == v) newDirection = RIGHT;
+				} else {
+					// Alternate order direction checking
+					if (*(pCurrent + 1) == v) newDirection = LEFT;
+					else if (*(pCurrent - 1) == v) newDirection = RIGHT;
+					else if (*(pCurrent - DECODED_PATHS_WIDTH) == v) newDirection = DOWN;
+					else if (*(pCurrent + DECODED_PATHS_WIDTH) == v) newDirection = UP;
+				}
+				if (newDirection == NO_DIRECTION)
+					error("Path finding process failed");
 
-		altFlag = stageCtr == 1;
-		pCurrent = _pDest;
+				// Process for the specified direction
+				if (newDirection != currDirection) add(newDirection, 0);
 
-		numSteps = 0;
-		currDirection = NO_DIRECTION;
-		while (1) {
-			v = *pCurrent - 1;
-			if (v == 0) break;
-
-			newDirection = NO_DIRECTION;
-			if (!altFlag && (currDirection != LEFT) && (currDirection != RIGHT)) {
-				// Standard order direction checking
-				if (*(pCurrent - DECODED_PATHS_WIDTH) == v) newDirection = DOWN;
-				else if (*(pCurrent + DECODED_PATHS_WIDTH) == v) newDirection = UP;
-				else if (*(pCurrent + 1) == v) newDirection = LEFT;
-				else if (*(pCurrent - 1) == v) newDirection = RIGHT;
-			} else {
-				// Alternate order direction checking
-				if (*(pCurrent + 1) == v) newDirection = LEFT;
-				else if (*(pCurrent - 1) == v) newDirection = RIGHT;
-				else if (*(pCurrent - DECODED_PATHS_WIDTH) == v) newDirection = DOWN;
-				else if (*(pCurrent + DECODED_PATHS_WIDTH) == v) newDirection = UP;
-			}
-			if (newDirection == NO_DIRECTION)
-				error("Path finding process failed");
-
-			// Process for the specified direction
-			if (newDirection != currDirection) add(newDirection, 0);
+				switch (newDirection) {
+				case UP:
+					pCurrent += DECODED_PATHS_WIDTH;
+					break;
 
-			switch (newDirection) {
-			case UP:
-				pCurrent += DECODED_PATHS_WIDTH;
-				break;
+				case DOWN:
+					pCurrent -= DECODED_PATHS_WIDTH;
+					break;
 
-			case DOWN:
-				pCurrent -= DECODED_PATHS_WIDTH;
-				break;
+				case LEFT:
+					++pCurrent;
+					break;
 
-			case LEFT:
-				++pCurrent;
-				break;
+				case RIGHT:
+					--pCurrent;
+					break;
 
-			case RIGHT:
-				--pCurrent;
-				break;
+				default:
+					break;
+				}
 
-			default:
-				break;
+				++numSteps;
+				top().rawSteps() += 8;
+				currDirection = newDirection;
 			}
 
-			++numSteps;
-			top().rawSteps() += 8;
-			currDirection = newDirection;
+			if (stageCtr == 0)
+				// Save the number of steps needed
+				savedSteps = numSteps;
+			if ((stageCtr == 1) && (numSteps <= savedSteps))
+				// Less steps were needed, so break out
+				break;
 		}
 
-		if (stageCtr == 0)
-			// Save the number of steps needed
-			savedSteps = numSteps;
-		if ((stageCtr == 1) && (numSteps <= savedSteps))
-			// Less steps were needed, so break out
-			break;
-	}
-
-	// Add final movement if necessary
+		// Add final movement if necessary
 
-	if (result == PF_OK) {
-		if (_xDestPos < 0)
-			addBack(LEFT, -_xDestPos);
-		else if (_xDestPos > 0)
-			addBack(RIGHT, _xDestPos);
+		if (result == PF_OK) {
+			if (_xDestPos < 0)
+				addBack(LEFT, -_xDestPos);
+			else if (_xDestPos > 0)
+				addBack(RIGHT, _xDestPos);
+		}
 	}
 
-final_step:
+	// Final Step
 	if (_xPos < 0) add(RIGHT, -_xPos);
 	else if (_xPos > 0) add(LEFT, _xPos);
 
diff --git a/engines/lure/menu.cpp b/engines/lure/menu.cpp
index 9919471c76..61de2bf165 100644
--- a/engines/lure/menu.cpp
+++ b/engines/lure/menu.cpp
@@ -515,7 +515,9 @@ uint16 PopupMenu::Show(int numEntries, const char *actions[]) {
 	r.top = Surface::textY();
 	r.bottom = s->height() - Surface::textY() + 1;
 
-	for (;;) {
+	bool bailOut = false;
+
+	while (!bailOut) {
 		if (refreshFlag) {
 			// Set up the contents of the menu
 			s->fillRect(r, bgColor);
@@ -546,8 +548,8 @@ uint16 PopupMenu::Show(int numEntries, const char *actions[]) {
 		while (e.pollEvent()) {
 			if (engine.shouldQuit()) {
 				selectedIndex = 0xffff;
-				goto bail_out;
-
+				bailOut = true;
+				break;
 			} else if (e.type() == Common::EVENT_WHEELUP) {
 				// Scroll upwards
 				if (selectedIndex > 0) {
@@ -571,10 +573,12 @@ uint16 PopupMenu::Show(int numEntries, const char *actions[]) {
 					++selectedIndex;
 					refreshFlag = true;
 				} else if ((keycode == Common::KEYCODE_RETURN) || (keycode == Common::KEYCODE_KP_ENTER)) {
-					goto bail_out;
+					bailOut = true;
+					break;
 				} else if (keycode == Common::KEYCODE_ESCAPE) {
 					selectedIndex = 0xffff;
-					goto bail_out;
+					bailOut = true;
+					break;
 				}
 
 #ifdef LURE_CLICKABLE_MENUS
@@ -586,46 +590,51 @@ uint16 PopupMenu::Show(int numEntries, const char *actions[]) {
 				if (r.contains(x, y)) {
 					selectedIndex = (y - r.top) / FONT_HEIGHT;
 					if (e.type() == Common::EVENT_LBUTTONDOWN)
-						goto bail_out;
+						bailOut = true;
+						break;
 				}
 #else
 			} else if ((e.type() == Common::EVENT_LBUTTONDOWN) ||
 					(e.type() == Common::EVENT_MBUTTONDOWN)) {
 				//mouse.waitForRelease();
-				goto bail_out;
+				bailOut = true;
+				break;
 #endif
 			} else if (e.type() == Common::EVENT_RBUTTONDOWN) {
 				mouse.waitForRelease();
 				selectedIndex = 0xffff;
-				goto bail_out;
+				bailOut = true;
+				break;
 			}
 		}
 
+		if (!bailOut) {
 #ifndef LURE_CLICKABLE_MENUS
-		// Warping the mouse to "neutral" even if the top/bottom menu
-		// entry has been reached has both pros and cons. It makes the
-		// menu behave a bit more sensibly, but it also makes it harder
-		// to move the mouse pointer out of the ScummVM window.
-
-		if (mouse.y() < yMiddle - POPMENU_CHANGE_SENSITIVITY) {
-			if (selectedIndex > 0) {
-				--selectedIndex;
-				refreshFlag = true;
-			}
-			mouse.setPosition(FULL_SCREEN_WIDTH / 2, yMiddle);
-		} else if (mouse.y() > yMiddle + POPMENU_CHANGE_SENSITIVITY) {
-			if (selectedIndex < numEntries - 1) {
-				++selectedIndex;
-				refreshFlag = true;
+			// Warping the mouse to "neutral" even if the top/bottom menu
+			// entry has been reached has both pros and cons. It makes the
+			// menu behave a bit more sensibly, but it also makes it harder
+			// to move the mouse pointer out of the ScummVM window.
+
+			if (mouse.y() < yMiddle - POPMENU_CHANGE_SENSITIVITY) {
+				if (selectedIndex > 0) {
+					--selectedIndex;
+					refreshFlag = true;
+				}
+				mouse.setPosition(FULL_SCREEN_WIDTH / 2, yMiddle);
+			} else if (mouse.y() > yMiddle + POPMENU_CHANGE_SENSITIVITY) {
+				if (selectedIndex < numEntries - 1) {
+					++selectedIndex;
+					refreshFlag = true;
+				}
+				mouse.setPosition(FULL_SCREEN_WIDTH / 2, yMiddle);
 			}
-			mouse.setPosition(FULL_SCREEN_WIDTH / 2, yMiddle);
-		}
 #endif
 
-		system.delayMillis(20);
+			system.delayMillis(20);
+		}
 	}
 
-bail_out:
+	// bailOut
 	delete s;
 
 #ifndef LURE_CLICKABLE_MENUS