Conversation
rhaps0dy
left a comment
rhaps0dy
left a comment
There was a problem hiding this comment.
Suggestions for improvement. The truly required ones are the -> ones.
envpool/sokoban/level_loader.h
Outdated
| int verbose; | ||
|
|
||
| std::vector<SokobanLevel>::iterator GetLevel(std::mt19937& gen); | ||
| std::pair<std::vector<std::pair<int, SokobanLevel>>::iterator, int> GetLevel( |
There was a problem hiding this comment.
I know this is a relatively simple data structure ( tuple[list[tuple[int, Level]], int] ), but I think this would benefit a lot from having names. Probably what I'd do is:
struct TaggedSokobanLevel {
int file_idx, level_idx;
SokobanLevel data;
}which would make it much easier to parse what's the output of this and what's going on in level_loader.cc.
Also minimizes refactoring. It would also be a possibility to name this struct SokobanLevel and make the original one SokobanLevelData. Idk.
rhaps0dy
left a comment
rhaps0dy
left a comment
There was a problem hiding this comment.
LGTM -- though see tips for avoiding copies
I think the other SokobanLevel level = ...GetLevel(). are not copies because of this https://stackoverflow.com/questions/14856344/when-should-stdmove-be-used-on-a-function-return-value
2 participants
No description provided.