It took me a while to figure out what you were really trying to do.
The variable names help. :) But you're making things much, much too complicated.
The idea is:
Find the current level.If it is not the last in the list: Assign the next element of the list to nextLevel.If it is not the first in the list: Assign the previous element of the list to prevLevel.
We don't need two separate loops for that. We don't need any loops, because there is a standard library algorithm for the first part.
Also, we should probably check that the current level is actually in the list before we try any of the rest. Your existing algorithm would not set 'nextLevel' or 'previousLevel' in this case, but would set the 'exists' flag and break out of the loop. This is probably not what we want.
We can also simplify loops (reduce indentation) with judicious use of 'continue'.
That gives us something like:
void setActiveLevel(const std::string level) { bool exists = false; std::map<std::string, boost::shared_ptr<LevelManager> >::iterator i; std::list<std::string>::iterator j; std::list<std::string>::reverse_iterator k; typedef std::list<std::string>::iterator level_it; for (i = mManagers.begin(); i != mManagers.end(); ++i) { if (i->first != level || levelNames.empty()) { continue; } level_it begin = levelNames.begin(), end = levelNames.end(); level_it current = std::find(begin, end, level); if (current == end) { continue; } exists = true; if (current != begin) { previousLevel = *(current - 1); } if (current + 1 != end) { nextLevel = *(current + 1); } break; } if (!exists) { std::cout << "Error setting the active level! Name does not exist!\n"; } lastVisitedLevel = activeLevel; activeLevel = level; std::cout << "nextLevel: " << nextLevel << std::endl; std::cout << "previousLevel: " << previousLevel << std::endl; std::cout << "lastVisitedLevel: " << lastVisitedLevel << std::endl; std::cout << "activeLevel: " << activeLevel << std::endl;}
But now we notice that 'levelNames' is a constant, and if it's empty, it will be empty on every iteration. So what we really want to do is check that first, and then we are simply searching through the map of managers to see if the 'level' is present. Each time through the loop, we do no real work if the level does not match, so really the loop has no purpose except to check for presence in the map. So we do that separately, too; and we again do it using built-in standard library functionality. (To search for a key in a std::map, we use the map's .find() member function, not the global std::find(), which would look for a key-value pair.)
We also should not be doing I/O here (I assume that stuff at the bottom is just for debugging); if there is an error, we should throw an exception or return some kind of error code. We currently do not have any normal value to return, so the natural thing to do is just "return whether successful".
Finally, we can clean up a couple of unused variables, pass the level name by const reference, etc.
// This doesn't really need any comments; I'm just putting them in to show you// how simple this really is.bool setActiveLevel(const std::string& level) { // Check if there is a manager for the named level. if (mManagers.find(level) == mManagers.end()) { return false; } // Check that the level name is in the list. // We don't actually need a separate check for an empty list, because // an empty list cannot contain the level name. ;) level_it begin = levelNames.begin(), end = levelNames.end(); level_it current = std::find(begin, end, level); if (current == end) { return false; } // All the checks passed, so we can set the level successfully. // Set the previous/next level names, if possible. if (current != begin) { previousLevel = *(current - 1); } if (current + 1 != end) { nextLevel = *(current + 1); } // Finish up. lastVisitedLevel = activeLevel; activeLevel = level; return true;}
