-
-
Notifications
You must be signed in to change notification settings - Fork 490
Use the ReportLogID enum instead of magic numbers for AddReportLog. #4407
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This makes code much more readable, gj |
This comment was marked as off-topic.
This comment was marked as off-topic.
Even if it's best practise, it will reduce ease and comfort of maintaining because of more steps & understanding being involved when adding new addReportLog lines in the future (Also more time, if you need to add multiple log lines to new code). Besides, there are some too generic keywords in the Enums list for this PR that'll make a global source search (to find the relevant print and its code) quite difficult if what you have is a report ID. Most of these also have no comment on their origin. Similarly, it is hard to consistently enforce that contributors will not choose generic/short keywords for new ID's (or alternatively, comment their source) and make that effect even worse. I'd say the ease with which is one reason to justify magic numbers in this case.. |
Let’s not exaggerate. If someone knows C++, adding a new value in an enum won’t be any problem. The time cost isn’t really significant either - features in VS like IntelliSense or Copilot suggest what to type after just 2–3 characters. As for wording and comments, that’s all negotiable. It’s not an issue to adapt to general requirements and, for example, add a note about where each value is used. But honestly, it seems simpler to search in code for a specific enum name rather than a random number like 7140 to find the spots where that log ID is used. Still, as I said, adding comments is no problem if needed (but it's not a good idea). If you prefer searching only by ID, that’s also fine - just look into ReportLogID.h where all IDs are defined. Pick the keyword assigned to your ID, right-click and choose Find All References - you instantly get the full list of usages and can jump there with a double click. On the matter of enforcing short names: AddReportLog isn’t something developers use frequently. It’s mainly there for debugging, in case of issues, to track what happened. And since every PR needs approval, enforcing naming quality shouldn’t really be a challenge. Introducing such an enum not only significantly improves readability and brings the code closer to modern standards, but also has practical benefits.
I don’t see any cons here. In a project of this size, using arbitrary numbers is simply not sustainable. That’s why this PR was created, and in my opinion such practices should be abandoned as soon as possible. As for netc, we could leave an overloaded version of the function with the old syntax specifically for netc, while the public code would use the new one with the enum. |
No description provided.