Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
FileEX wants to merge 3 commits into multitheftauto:master
base: master
Choose a base branch
Loading
from FileEX:refactor/report_log_id

Conversation

@FileEX
Copy link
Member

@FileEX FileEX commented Sep 1, 2025

No description provided.

Copy link
Contributor

This makes code much more readable, gj

This comment was marked as off-topic.

Copy link
Member

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..
I am against it, the benefits aren't worth the cons.

Copy link
Member Author

FileEX commented Sep 11, 2025
edited
Loading

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.. I am against it, the benefits aren't worth the cons.

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.

  • Simpler searching: with enums, you can instantly find every usage of a value project-wide in 2–3 clicks. With plain numbers, you’d need to do a slower text search like AddReportLog(number.

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.

Copy link
Contributor

@Pirulax Pirulax left a comment
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

I personally see no cons here either. To answer Dutchman's points:

it will reduce ease and comfort of maintaining because of more steps & understanding being involved when adding new addReportLog lines in the future

Even if that was the case, let's be realistic, this enum won't be changed often at all...

make a global source search (to find the relevant print and its code) quite difficult if what you have is a report ID

As FileEX said this doesn't stand grounds either, because even GitHub now supports search by just clicking over some definition, like the enum's.... Any half decent IDE has such search functionality.

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.

Doesn't matter how short the ID is, since it's a scoped enum, the ReportLogID:: prefix will always be there, so any kind of string search is gonna be quite easy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Pirulax Pirulax Pirulax approved these changes

@Dutchman101 Dutchman101 Awaiting requested review from Dutchman101

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /