Conversation
Sources/Helpers/Chat.cpp
Outdated
| void Chat::CommandLoop() { | ||
|
|
||
| //macro to clean up | ||
| #define CLEAR_RET \ |
There was a problem hiding this comment.
This is not a good idea. It hides the return in the other lines, making the function less readable, the user won't know if the function returns there unless they look at the macro.
And performance wise a macro is useless, its contents get pasted into its declarations while compiling. At the end, it just makes the code less readable. Readability is very important. But I do have to admit that this function is not very clean. Maybe using lambdas might make it cleaner, it does go in a similar direction as your macro
There was a problem hiding this comment.
You could do something like this:
auto notifyAndClear = [this](TextID id) {
OSDExtras::Notify(id, Color::Red);
this->ClearPlayerMessage();
};And then just do:
if (!IDList::AnimationValid(animID, GetPlayerIndex())) {
return notifyAndClear(TextID::CHAT_INVALID_ANIMATION);
}This would make it clear what is happening
There was a problem hiding this comment.
Understood, no disgustingly ugly macro definitions allowed here 👍
tbh, I thought the macro was ugly too but I didn't want to have duplicate code. A lambda indeed seems more like a c++ism. Thanks.
more functions, less nesting
|
Here I have cleaned the function to hopefully make it more readable. each way to set ID has been moved to its own function and we don't even need to put |
Sources/Helpers/Chat.cpp
Outdated
| return false; | ||
| } | ||
| ItemCommand(); | ||
| return false; |
There was a problem hiding this comment.
I am guessing this was by accident? Should be return true;
There was a problem hiding this comment.
Yep that’s a mistake 😓
|
Here I fixed that return. |
fix my previous edits to clear message when there is an invalid ID
also fix my commenting style to match this project