15
What further improvements can I make to this code?
The code is rather bad - the site you read it from tends to have lots of bad/amateur articles.
The graph class:
std::map<int, bool> visited;
std::map<int, std::list<int>> adj;
This is extremely poor and inefficient graph implementation. std::map creates a new memory allocation for each ...
10
Disagreement
I have to disagree with @ALX23z blanket statement that using std::map is bad idea. There is a lot more to it than that (if the graph was sparse the map is great). But you don't give enough contect to evaluate the graph implementation. All I can say it is an acceptable "basic" implementation.
Overall
Things that are actually bad:
1: The ...
9
I've assumed here the OP is performing DFS on undirected graphs - if not only my second point is valid.
There are several other answers here suggesting better coding practices and more efficient implementations, but they're missing the biggest problem with you code: it doesn't perform depth first search!
To demonstrate this run it with the graph created by:
...
4
Looks pretty good at first glance! I'd say your main problem is "#define ONE 1" — you've gone out of your way to parameterize things that can't actually be changed in real life without massive changes elsewhere in the game logic. For example, you've set
enum Items { Rock, Paper, Scissors, ItemsCount };
which looks awesome... until someone decides ...
4
std::map<int, bool> visited;
This is useless as a member variable.
It means nothing before or after you call DFS, so it's a waste of space most of the time
There's only one copy of it for a given Graph, which means you can't call DFS twice concurrently
It retains its state after calling DFS the first time, which means you can't even call DFS twice in ...
Only top voted, non community-wiki answers of a minimum length are eligible
