DefaultComesLast: new option skipIfLastAndSharedWithCase to raise violation if default doesn't share case #4078

Closed
robertpainsi opened this Issue Mar 21, 2017 · 5 comments

Comments

Projects
None yet
4 participants
@robertpainsi
Contributor

robertpainsi commented Mar 21, 2017

The DefaultComesLast check should have a new option to allow the default case not to be the last one if it shares another case. e.g.

$ javac MyClass.java 
$ cat config.xml 
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
<module name="Checker">
    <property name="charset" value="UTF-8"/>
    <property name="severity" value="warning"/>
    <module name="TreeWalker">
      <module name="DefaultComesLast"/>
    </module>
</module>

$ cat MyClass.java 
public class MyClass {
    int mode = 4;
    int method2() {
        switch (mode) {
            case 1:
            default: // No violation with the new option is expeced
                return 1;
            case 2:
                return 2;
            case 3:
                return 3;
        }
     }
}

$ java -jar checkstyle-7.6-all.jar -c config.xml MyClass.java 
Starting audit...
[WARN] /var/tmp/MyClass.java:6:13: Default should be last label in the switch. [DefaultComesLast]
Audit done.

The default statement shares case 1. This is useful if the cases are named (e.g enum) or have an order (e.g. natural, alphabetically). However, if the default case doesn't share a case implementation, it should be at the last position.

@sagar-shah94

This comment has been minimized.

Show comment
Hide comment
@sagar-shah94

sagar-shah94 Mar 22, 2017

Contributor

I am on it

Contributor

sagar-shah94 commented Mar 22, 2017

I am on it

@rnveach rnveach added new feature and removed new feature labels Mar 22, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 22, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 22, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 22, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 23, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 23, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 23, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 24, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 24, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 24, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 24, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 25, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 25, 2017

Member

This issue was approved only because there is request for new property to allow user case.

Without new property such update damage whole idea of the check.
I personally do not like default to be somewhere in the middle - it is hard to catch when switch is big and have big blocks for cases. In checkstyle code this property will never be used.

Author of request might be right in his context (which I do not know), and agree that there might be cases like this where it make sense. So I agree on new property.

my proposal for property name: skipIfLastAndSharedWithCase
@robertpainsi , @rnveach - please propose better name if you have.

use cases:

        switch (mode) {
            case 1:
            default: // No violation with the new option is expeced
                return 1;
        }
        switch (mode) {
            default: // violation !!!! with the new option is expected
            case 1:
                return 1;
        }
Member

romani commented Mar 25, 2017

This issue was approved only because there is request for new property to allow user case.

Without new property such update damage whole idea of the check.
I personally do not like default to be somewhere in the middle - it is hard to catch when switch is big and have big blocks for cases. In checkstyle code this property will never be used.

Author of request might be right in his context (which I do not know), and agree that there might be cases like this where it make sense. So I agree on new property.

my proposal for property name: skipIfLastAndSharedWithCase
@robertpainsi , @rnveach - please propose better name if you have.

use cases:

        switch (mode) {
            case 1:
            default: // No violation with the new option is expeced
                return 1;
        }
        switch (mode) {
            default: // violation !!!! with the new option is expected
            case 1:
                return 1;
        }

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 25, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 26, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 27, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 27, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 27, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 29, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 29, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 29, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 29, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 29, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 29, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 29, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 29, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Apr 4, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Apr 4, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Apr 5, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Apr 5, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Apr 8, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Apr 8, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Apr 8, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Apr 8, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Apr 8, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Apr 9, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Apr 9, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Apr 9, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Apr 9, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Apr 9, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Apr 10, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Apr 10, 2017

sagar-shah94 added a commit to sagar-shah94/checkstyle that referenced this issue Apr 10, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Apr 12, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Apr 12, 2017

sagar-shah94 added a commit to sagar-shah94/checkstyle that referenced this issue Apr 12, 2017

romani added a commit that referenced this issue Apr 12, 2017

@romani romani added the new feature label Apr 12, 2017

@romani romani added this to the 7.7 milestone Apr 12, 2017

@romani romani changed the title from DefaultComesLast only if doesn't share case to DefaultComesLast: new option skipIfLastAndSharedWithCase to raise violation if default doesn't share case Apr 12, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 12, 2017

Member

fix is merged

Member

romani commented Apr 12, 2017

fix is merged

@romani romani closed this Apr 12, 2017

@robertpainsi

This comment has been minimized.

Show comment
Hide comment
@robertpainsi

robertpainsi Apr 17, 2017

Contributor

@sagar-shah94, @romani, @rnveach, tested it, works like a charm! Thank you very much! 😘

Contributor

robertpainsi commented Apr 17, 2017

@sagar-shah94, @romani, @rnveach, tested it, works like a charm! Thank you very much! 😘

@sagar-shah94

This comment has been minimized.

Show comment
Hide comment
@sagar-shah94

sagar-shah94 Apr 17, 2017

Contributor

@robertpainsi Glad to know that.
Thanks .

Contributor

sagar-shah94 commented Apr 17, 2017

@robertpainsi Glad to know that.
Thanks .

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