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

Remove StrictDuplicateCodeCheck and whole package #523

Closed
romani opened this issue Dec 19, 2014 · 7 comments
Closed

Remove StrictDuplicateCodeCheck and whole package #523

romani opened this issue Dec 19, 2014 · 7 comments
Labels

Comments

@romani
Copy link
Member

romani commented Dec 19, 2014

remove whole com.puppycrawl.tools.checkstyle.checks.duplicates package from Checkstyle.

Reason:
searching duplicate code by strict code comparison is the most error prone approach in static code analysis, as created dependency between classes (caused by following Chckstyle instructions) is more problematic that code duplication. Another reason is that identical code does not mean that it describe the same and could be shared.

@romani
Copy link
Member Author

romani commented Dec 20, 2014

will be released in 6.2.

@romani romani closed this as completed Dec 20, 2014
mkordas added a commit to mkordas/checkstyle that referenced this issue Dec 20, 2014
mkordas added a commit to mkordas/checkstyle that referenced this issue Dec 20, 2014
@lkoe
Copy link
Member

lkoe commented Dec 31, 2014

Personally I think that's a really rash decision, with some debatable reasoning attached.

If I understand correctly, you prefer verbatim copy&paste of code over proper code design and reuse of common functionality by delegation.
StrictDuplicateCodeCheck was a simple but efficient tool to discover such verbatim duplications, which are indeed dangerous (duplicated bugs, unawareness of duplication when extending original functionality etc. these arguments and more are well established for decades).

Of course there might be a low proportion of code duplications which may be legitimate (or even "false" positives) - but those can be easily handled by using suppression facilities of Checkstyle.

Another factor in making such decisions should also be the established user and configuration base of Checkstyle. Every time a check (and even one that is not particularly broken) is removed, thousands of Checkstyle configurations out there will be broken when updating, alienating users and stripping them of well established functionality - for in my opinion questionable reasoning.

Therefore I vote for re-adding this well established and useful check.

@romani
Copy link
Member Author

romani commented Jan 1, 2015

Personally I think that's a really rash decision, with some debatable reasoning attached.

I keep that thought for one year, and persuaded myself to make that breaking compatibility after java8 support is introduced, after that I did few more releases ….. so that is last time I could wait to remove buggy stuff from Checkstyle.

If I understand correctly, you prefer verbatim copy&paste of code over proper code design and reuse of common functionality by delegation.
StrictDuplicateCodeCheck was a simple but efficient tool to discover such verbatim duplications, which are indeed dangerous (duplicated bugs, unawareness of duplication when extending original functionality etc. these arguments and more are well established for decades).

implementation is simple, that is why it is full of bugs and false positives. It is not efficient !!!! It is useless generator of violations. Nobody use that Check as to work with it is too hard. If smb do use it - he should stop live in illusion and look around for better alternatives.

Of course there might be a low proportion of code duplications which may be legitimate (or even "false" positives) - but those can be easily handled by using suppression facilities of Checkstyle.

Suppression is Chekcstyle is not a simple option, even I have problems sometime to suppress violations ( but this is a different update for Checkstyle and plugins to ease that).

Another factor in making such decisions should also be the established user and configuration base of Checkstyle. Every time a check (and even one that is not particularly broken) is removed, thousands of Checkstyle configurations out there will be broken when updating, alienating users and stripping them of well established functionality - for in my opinion questionable reasoning.

That Check produce so much noise that I doubt that anybody use it. And even they use it - they need consider to use smth else. They need to use CPD of PMD for that it works much better and far from simple. But even it far from usable. By means on “usable“ I mean not be crazy with configurations. All attempts to show similar code in non static methods - is false positive in 90%, all further attempts to make system without code duplication is wrong way in design as it introduce dependencies that are much harder to fix rather than code duplication.
Checkstyle will NOT be a container of immature and buggy validations, it was such tool for long time, but not any more. All experiments will be removed from Checkstyle. You might be noticed that the same was happen with “TypeAware” Checks.

Therefore I vote for re-adding this well established and useful check.

thanks for your vote, if I found that smb else(about a bunch of people) is desperately miss that check, I agree to move it to our experimental project - sevntu.checkstyle. But there is no reason to keep in stable Checkstyle.

@netmikey
Copy link

We actually are using it (or should I say were?) and quite happy with it. I have to agree with @lkoe - guess why I landed here? ;) A simple Eclipse update spawned errors in the checkstyle builder, which had been updated to 6.2.

I'll have to go through all our project checkstyle configurations and remove that now... It's one thing to deprecate a feature and remove it from default configuration templates, but it's another to break things like this - even with the best intentions.

@romani
Copy link
Member Author

romani commented Feb 20, 2015

@netmikey, if you need StrictDuplicateCodeCheck so much please move it to sevntu-checkstyle project, our official sandbox project (https://github.com/sevntu-checkstyle/sevntu.checkstyle).

but it's another to break things like this - even with the best intentions.

Bracking things is not good , but that is the only way for Checkstyle to survive as quality of Checkstyle's code is very poor and I will fix the Check if that possible. If Chck idea is incomplete - Check should go to trash or experimental project like sevntu-checkstyle to get maturity.

Right now I expect small bracking compatibility in each release for the next year (I plan to have release at the end of each month). Sorry, that is a payment for several years of lacking time to think and throughly test Checks before introducing them.

@ewoks
Copy link

ewoks commented Feb 23, 2015

Did u plan some replacement? Some more efficient error and false positive prone solution? In my project CheckStyle was marking crazy stuff as duplicate, code that doesn't have to do with each other at all so i am happy bad implementation is gone, but i would like to have some better solution inside CheckStyle. Thanks

@romani
Copy link
Member Author

romani commented Feb 24, 2015

no replacements is planned as I have only some ideas how to make code duplication non error prone.
I you have some or you have time to do implementation - you are welcome.

I really recommend to not focus that much on code duplication and focus on minimizing dependencies between code, look at Data Structure Matrix report against Chekstyle - http://checkstyle.sourceforge.net/dsm/index.html it shows design problems . By following guidelines from duplicate code detector - you force your application to introduce dependencies that will not do any good to your code.

tkashi pushed a commit to hifive/hifive-pitalium that referenced this issue Aug 24, 2015
StrictDuplicateCodeCheckが6.4から廃止されている
checkstyle/checkstyle#523

Check Stykeを最新にしたことに伴い、設定を無効化
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants