Skip to content
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

feat: abstract ruletype enum with IRuleType #759

Merged
merged 3 commits into from
Mar 25, 2022

Conversation

MartinWitt
Copy link
Contributor

Currently, the RuleType enum is used in alot of places. But there could exist more categories of bad smells than the 3 sonar ones. Using an interface in the code allows adding new categories easier, e.g., from other analyzers, without constantly changing the enum.

@algomaster99 algomaster99 self-requested a review March 25, 2022 10:23
Copy link
Member

@algomaster99 algomaster99 left a comment

Choose a reason for hiding this comment

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

Having enums inherit from an interface is a good idea to segregate rule types from different analyzers. We can proceed with this. Look at my comments.

src/main/java/sorald/rule/RuleType.java Outdated Show resolved Hide resolved
src/main/java/sorald/sonar/SonarRule.java Show resolved Hide resolved
Comment on lines 5 to 8
BUG("Bug"),
VULNERABILITY("Vulnerability"),
CODE_SMELL("Code_Smell"),
SECURITY_HOTSPOT("Security_Hotspot");
Copy link
Member

@algomaster99 algomaster99 Mar 25, 2022

Choose a reason for hiding this comment

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

Any specific reason to pass an argument to the constructor? We can use name method of Enum to get the string representation which will default to "BUG" and so on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name method always returns the name in uppercase and is not really changeable. Using a String would allow us to change the name(the String) in the future. Furthermore, printing these is way easier because you don't need a lot of String manipulations. For example, you would print Security_Hotspot more likely than SECURITY_HOTSPOT. Converting SECURITY_HOTSPOT to a user-friendly, readable version requires unnecessary complex logic.

Copy link
Member

Choose a reason for hiding this comment

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

The name method always returns the name in uppercase and is not really changeable.

Sounds good.

Furthermore, printing these is way easier because you don't need a lot of String manipulations

If it is supposed to be an output for user, it is better to name it toString than getName.

Comment on lines +5 to +8
/**
* Returns the name of the rule type. A rule type is a category of rules, such as "Bug",
* "Vulnerability".
*/
Copy link
Member

Choose a reason for hiding this comment

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

This is sufficient, I believe.

@algomaster99 algomaster99 merged commit 59667b5 into ASSERT-KTH:spi Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants