Config standardization #74
Conversation
|
|
||
|
|
||
|
|
||
| bool LegacyLadderConfig::ParseConfig() |
There was a problem hiding this comment.
I vote for removal of all the legacy code. There are two reasons behind:
a. We have version control history so can always revert the changes (especially if it would be in a single commit).
b. Current implementation of the cmake routine will grab all the source files, including the legacy code, which means that we have to support this code in the future.
If you still want to keep both implementations, then probably we should use a kind of virtual interface class in order to force both implementation use the same interface and implement it in some way. That will allow to easily switch to a different config in the future.
There was a problem hiding this comment.
I would also like to remove it as well, but I kept it in because it was easy enough, just in case we decided to support backwards compatibility.
If no-one else has any qualms, I'll just remove it.
|
|
||
| #include <string> | ||
| #include <map> | ||
|
|
||
| class LadderConfig | ||
| { | ||
| public: |
There was a problem hiding this comment.
Please use explicit in the constructor.
There was a problem hiding this comment.
Any specific reason for this? or is this just good practice?
Either way, happy to do this.
There was a problem hiding this comment.
There are number of caveats behind, see:
https://stackoverflow.com/questions/12437241/c-always-use-explicit-constructor/12437453
|
I'll resolve any merge conflicts when I get a chance. |
|
Changes pushed. |
| @@ -0,0 +1,12 @@ | |||
| { | |||
| "CommandCenterPath": "./path/to/CommandCenter.exe", | |||
| "LocalReplayDirectory": "./Replays/", | |||
There was a problem hiding this comment.
Btw we should fix this trailing slash somehow cause the server doesn't work without it.
Of course, not in this PR.
There was a problem hiding this comment.
Agreed - this is a good suggestion.
I will add a github issue for this.
src/sc2laddercore/LadderConfig.cpp
Outdated
| } | ||
| return true; | ||
| else |
There was a problem hiding this comment.
Style: else is not needed after return. Here and in other places.
There was a problem hiding this comment.
This is just the way I'm used to because I understand it easier - it tells me there should be a return somewhere in the if statement when I see the else return.
Is your suggestion a more standard way of doing it?
Regardless, not too fussed which way it is - just asking for my own edification.
There was a problem hiding this comment.
Nope, this is a matter of taste.
There was a problem hiding this comment.
No problem - I have updated it for you.
src/sc2laddercore/LadderConfig.cpp
Outdated
| { | ||
| Str.erase(p + 1); | ||
| if (doc[RequestedValue].IsString()) | ||
| return doc[RequestedValue].GetString(); |
There was a problem hiding this comment.
Should always have braces on an if statement, just makes things a lot cleaner and easier to read
Intended to resolve #71
Suggestions welcome.