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

Rewrite messages and issue description for TooManyFunctions rule - #552 #560

Merged
merged 2 commits into from Nov 19, 2017

Conversation

arturbosch
Copy link
Member

@arturbosch arturbosch commented Nov 18, 2017

Please also look at the other linked commit inside #552. I accidentally pushed the changes to master.
This PR adds messages for different TooManyFunctions findings and repairs the build.

Edit: I don't want to rewrite master history as someone may depend on the newest commit. Reverting the last commit can still be done if the changes are not good enought.

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Looks good! I think the new rule is definitely better than the old one.

Copy link
Collaborator

@Mauin Mauin left a comment

Choose a reason for hiding this comment

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

Maybe some small changes to the issue description, otherwise looks great!

"large classes and can quickly become God classes. " +
"Consider extracting methods to (new) classes better matching their responsibility.")
"Too many functions inside a/an file/class/object/interface always indicate a violation of "
+ "the single responsibility principle. Maybe your file/class/object/interface wants to manage to many " +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe the file/class/...

"Consider extracting methods to (new) classes better matching their responsibility.")
"Too many functions inside a/an file/class/object/interface always indicate a violation of "
+ "the single responsibility principle. Maybe your file/class/object/interface wants to manage to many " +
"things at once. Try to refactor out functionality which belong clearly together.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd change the last sentence to "Extract functionality which clearly belongs together."

@@ -56,20 +57,25 @@ class TooManyFunctions(config: Config = Config.empty) : Rule(config) {
report(ThresholdedCodeSmell(issue,
Entity.from(klass),
Metric("SIZE", amount, thresholdInInterfaces),
message = "")) // TODO: add class message
"Interface '${klass.name}' with '$amount' functions detected. " +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Totally love these messages and the descriptive issue description! 👍

@arturbosch
Copy link
Member Author

I included your suggestions, thanks for the reviews!

@arturbosch arturbosch merged commit 2461232 into master Nov 19, 2017
@arturbosch arturbosch deleted the toomanyfunctions-rework branch November 19, 2017 09:05
@arturbosch arturbosch added this to the RC5-5 milestone Nov 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants