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

Issue #374: Fix SingleBreakOrContinue violations #423

Closed
wants to merge 1 commit into from

Conversation

yaziza
Copy link
Member

@yaziza yaziza commented Jan 11, 2016

No description provided.

@yaziza
Copy link
Member Author

yaziza commented Jan 11, 2016

Update: 6/7 violations resolved.

Im still working on the last violation. After fixing i will extend the unit tests to meet the requirements.

if (isAnonymousClassField(ast)
|| classDetail.containsGetter(methodName)
|| classDetail.containsSetter(methodName)
|| isMainMethod(ast)) {
result = index;
break;
Copy link
Member Author

Choose a reason for hiding this comment

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

After reducing the number of breaks to 2, not sure how to eleminate the last one ...
Maybe completly refactoring this method ...

Copy link
Member

Choose a reason for hiding this comment

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

code is not simple :), this is just good example that need to follow new Checks ourself.

solution is to put result != null condition to FOR, and remove break completely.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, i didn't get it.

What about setting: index = Integer.MAX_VALUE - 1; instead f break ?

Copy link
Member

Choose a reason for hiding this comment

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

make loop like for (int index = 0; index < customOrderDeclaration.size() && result != 1 ; index++)
no breaks are required.

@romani
Copy link
Member

romani commented Jan 13, 2016

please remove from you commit all unrelated changes(that are just formatting), or move them in separate commit of this PR. This will ease diff verification.

@romani
Copy link
Member

romani commented Jan 13, 2016

Travis is not happy with reduced coverage

[ERROR] Jan 11, 2016 7:59:02 PM net.sourceforge.cobertura.coveragedata.CoverageDataFileHandler loadCoverageData
INFO: Cobertura: Loaded information on 63 classes.
com.github.sevntu.checkstyle.checks.coding.DiamondOperatorForVariableDefinitionCheck failed check. Branch coverage rate of 85.0% is below 90.0%

It might be resulted by removing the code lines. Please generate coverage report and share pictures of coverage before and after for this method. If it is the same it is ok to change configuration(see cobertura section pom.xml) there limit is specified.

@romani
Copy link
Member

romani commented Jan 16, 2016

@yaziza , 1.18 release will be this Sunday, please find time to finish this PR.

@yaziza
Copy link
Member Author

yaziza commented Jan 17, 2016

Fixed, i have added a new case in the test input file and the branch cover is now 94%
Also i have removed the unnesseray != null check, which will be checked anyway in equals.

Thank you for your support !

Do you have any suggestions on issues i can tackle next ?

@romani
Copy link
Member

romani commented Jan 17, 2016

merged as FF. Thanks a lot for your time and contributions.

Also i have removed the unnesseray != null check

One of the banefit of 100% coverage is to remove dead/extra code.

Do you have any suggestions on issues i can tackle next ?

Can you help us with #410 ? Exceptions during execution usually force user to deactivate the Check at all. But you can choose any unassigned issue.

@romani romani closed this Jan 17, 2016
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.

2 participants