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

[java] Add two linguistics rules under naming #109

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@ardaasln
Contributor

ardaasln commented Sep 12, 2016

No description provided.

Arda Aslan added some commits Sep 12, 2016

Arda Aslan Arda Aslan
Added a naming rule called IsDoesNotReturnBoolean. According to artic…
…le Linguistics antipatterns: what they are and how developers percieve them, a method starting with isX and not returning boolean introduces a code smell, reduces maintainability mainly. Feedbacks are appreciated
Arda Aslan Arda Aslan
Fixed RuleSetFactoryTest>AbstractRuleSetFactoryTest.testAllPMDBuiltIn…
…RulesMeetConventions:157 All built-in PMD rules need 'since' attribute (1 are missing), a proper ExternalURLInfo (1 are invalid), a class name meeting conventions (1 are invalid), no 'violationSuppressRegex' property (0 are invalid), and no 'violationSuppressXPath' property (0 are invalid)
@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Sep 12, 2016

Interesting!

Though it should probably be a little more broad to cover other scenarios than isXXX such as hasXXX, canXXX or willXXX... any thoughts?

@ardaasln

This comment has been minimized.

Contributor

ardaasln commented Sep 12, 2016

There are tens of linguistics anti-patterns that I am planning to implement. If you mean that we should combine all of them(hasXXX, canXXX etc...) to one rule it is fine. We can combine them into a rule “MethodDeclarationSaysDifferentThanItDoes”

Contents of this rule:
hasXXX, isXXX, canXXX, willXXX, Expecting but not getting a collection(getStats returning a object instead of a collection) ……
(If you have any other suggestion we can consider them)

There are many other anti-patterns that cannot be classified into this ruleset

For example:

  • Set method returns(it sets the attribute successfully but also returns)

On 12 Sep 2016, at 21:11, Juan Martín Sotuyo Dodero notifications@github.com wrote:

Interesting!

Though it should probably be a little more broad to cover other scenarios than isXXX such as hasXXX, canXXX or willXXX... any thoughts?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #109 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AVIDVPjxJSjopPwUjJLYtCRpQypLF3jiks5qpZXIgaJpZM4J63QE.

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Sep 12, 2016

True, I was just focusing on boolean methods though. Splitting other return types to separate rules seems the most reasonable thing to do, but having a rule per prefix is maybe a little too much.

Arda Aslan added some commits Sep 13, 2016

Arda Aslan Arda Aslan
Add two linguistics codes smells. one related with methods and the ot…
…her with variables (these two can be merged into 1 rule later)
Arda Aslan Arda Aslan

@ardaasln ardaasln changed the title from Add new Naming Rule IsDoesNotReturnBoolean to Add two linguistics rules under naming Sep 13, 2016

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Sep 13, 2016

Nice! You seem to be repeating too much code 'though. You can probably simplify greatly by doing something such as:

private static final Set<String> PREFFIXES;

static {
  final Set<String> preffixCollection = new HashSet<String>();
  preffixCollection.add("can");
  preffixCollection.add("is");
  preffixCollection.add("has");
  ....
  PREFFIXES = Collections.unmodifiableSet(preffixCollection);
}

and then...

for (final String preffix : PREFFIXES) {
  if (nameOfField.startsWith(preffix) && (nameOfField.charAt(preffix.length()) > 64) && (nameOfField.charAt(preffix.length()) < 91)) { // after preffix, next char a capital letter expected 
    if (!"boolean".equals(t.getType().getName())) {
      addViolation(data, node);
    }
    break;
  }
}
@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Sep 13, 2016

Also, you should check against the method name's length before calling charAt. A method simply called is(Class<?> clazz) would otherwhise throw an IndexOutOfBoundsException

@ardaasln

This comment has been minimized.

Contributor

ardaasln commented Sep 13, 2016

Nice call for using Set. Gonna do that.
About the second issue have never seen a method called just is. We need to assume programmer sticks to some coding standards. A method named isvalid also can cause problem where programmer means isValid but to be safe, surely, length check can be done

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Sep 13, 2016

@ardaasln I beleive there is a big difference between a false negative (FN) due to bad naming over having PMD break during a check with an exception for not being defensive in our coding.

We can live with FNs when we can't get rid of them, but we can't live with a failing tool.

@ardaasln

This comment has been minimized.

Contributor

ardaasln commented Sep 13, 2016

Exactly thats what I thought now. crash is not good anyways

On 13 Sep 2016, at 17:44, Juan Martín Sotuyo Dodero notifications@github.com wrote:

@ardaasln https://github.com/ardaasln I beleive there is a big difference between a false negative (FN) due to bad naming over having PMD break during a check with an exception for not being defensive in our coding.

We can live with FNs when we can't get rid of them, but we can't live with a failing tool.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #109 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AVIDVHk-XI6LJbEjYOG-wGNG1dokFWjiks5qprbHgaJpZM4J63QE.

@hooperbloob

This comment has been minimized.

Contributor

hooperbloob commented Sep 13, 2016

Also, don't hard-code the values, ensure users can edit/add their own via the preferences.  Using a StringMultiProperty is your best choice, you can supply your original items as default values.--- notifications@github.com wrote:From: ardaasln notifications@github.comTo: pmd/pmd pmd@noreply.github.comSubject: Re: [pmd/pmd] Add two linguistics rules under naming (#109)Date: Tue, 13 Sep 2016 07:23:47 -0700Nice call for using Set. Gonna do that.
About the second issue have never seen a method called just is. We need to assume programmer sticks to some coding standards. A method named isvalid also can cause problem where programmer means isValid but to be safe, surely, length check can be done

—You are receiving this because you are subscribed to this thread.Reply to this email directly, view it on GitHub, or mute the thread.

@adangel

This comment has been minimized.

Member

adangel commented Sep 25, 2016

Hi @ardaasln !

Thanks for contributing!
I think, the rules will be useful. There is only one similar rule existing yet - BooleanGetMethodName - but it only covers boolean (and only the get/is case).

Here are a couple of notes:

  • Please make sure, that the code compiles. Currently it has some compilation errors, probably caused the "wrong" quotes in naming.xml, and NamingRulesTest.java.
  • I would try to make the examples in naming.xml more concise. It's good to have the complete code in the test, but I think, it's not necessary to write out each case for the example documentation. I think, the following would be enough for MethodTypeAndNameIsInconsistent:

int isotherm() { ... } // ignored - doesn't match Java Beans getter name convention
// the following methods are examples of violations - they all should return boolean
int isValid() { ... }
int hasChild() { ... }
int haveLegs() { ... }
int canFly() { ... }
int willFly() { ... }
int shouldFly() { ... }
// setters shouldn't return anything
int setName(...) { ... }
// getters must return
void getName() { ... }

Similar for AttributeTypeAndNameIsInconsistent:

// the following fields are examples of violations - they all should be of type boolean
int isValid;
int hasMoney;
int canFly;
int willMove;
int haveLegs
int shouldClimb;

It's then enough to mention in the description that both instance variables and local variables are checked.

If we make the prefixes configurable via properties, then even less examples would be necessary.

  • Maybe I would rename AttributeTypeAndNameIsInconsistent to FieldTypeAndNameIsInconsistent or VariableType... It also seems, that the rule currently only highlights misnamed boolean variables. Maybe its name should even be "BooleanVariableNameIsInconsistent" in it? That would restrict the rule of course to boolean...
  • Making the prefixes for the boolean cases configurable via parameters makes perfect sense. E.g.

private StringMultiProperty BOOLEAN_PREFIXES = new StringMultiProperty("booleanPrefixes", "the prefixes of methods that return boolean",
new String[] {"is", "has", "can", "have", "will", "should"}, 1.0f, ',');
public MethodTypeAndNameIsInconsistentRule() {
definePropertyDescriptor(BOOLEAN_PREFIXES);
}

Then later on, you can use it like this:

    Set<String> prefixes = new HashSet<>(Arrays.asList(getProperty(BOOLEAN_PREFIXES)));

Let me know if you have any questions!
Regards,
Andreas

@ardaasln

This comment has been minimized.

Contributor

ardaasln commented Sep 25, 2016

Hello Andreas,

I will work on this and I have an additional question. As PMD website declares, there are 5 Android specific anti patterns. I am also thinking to extend it.There are many other android code smells where I think some of them can be implemented via PMD rules Does PMD work on any other Android specific anti patterns, if so, what are them.

On 25 Sep 2016, at 18:56, Andreas Dangel notifications@github.com wrote:

Hi @ardaasln https://github.com/ardaasln !

Thanks for contributing!
I think, the rules will be useful. There is only one similar rule existing yet - BooleanGetMethodName http://pmd.github.io/pmd-5.5.1/pmd-java/rules/java/naming.html#BooleanGetMethodName - but it only covers boolean (and only the get/is case).

Here are a couple of notes:

Please make sure, that the code compiles. Currently it has some compilation errors, probably caused the "wrong" quotes in naming.xml https://github.com/pmd/pmd/pull/109/files#diff-a39ce976d0820cebc9d6410a93e7e513R600, and NamingRulesTest.java https://github.com/pmd/pmd/pull/109/files#diff-71e7c9e6dc1757d955edfcccb0dd46c8R34.

I would try to make the examples in naming.xml more concise. It's good to have the complete code in the test, but I think, it's not necessary to write out each case for the example documentation. I think, the following would be enough for MethodTypeAndNameIsInconsistent:

int isotherm() { ... } // ignored - doesn't match Java Beans getter name convention
// the following methods are examples of violations - they all should return boolean
int isValid() { ... }
int hasChild() { ... }
int haveLegs() { ... }
int canFly() { ... }
int willFly() { ... }
int shouldFly() { ... }
// setters shouldn't return anything
int setName(...) { ... }
// getters must return
void getName() { ... }

Similar for AttributeTypeAndNameIsInconsistent:

// the following fields are examples of violations - they all should be of type boolean
int isValid;
int hasMoney;
int canFly;
int willMove;
int haveLegs
int shouldClimb;

It's then enough to mention in the description that both instance variables and local variables are checked.

If we make the prefixes configurable via properties, then even less examples would be necessary.

Maybe I would rename AttributeTypeAndNameIsInconsistent to FieldTypeAndNameIsInconsistent or VariableType... It also seems, that the rule currently only highlights misnamed boolean variables. Maybe its name should even be "BooleanVariableNameIsInconsistent" in it? That would restrict the rule of course to boolean...

Making the prefixes for the boolean cases configurable via parameters makes perfect sense. E.g.

private StringMultiProperty BOOLEAN_PREFIXES = new StringMultiProperty("booleanPrefixes", "the prefixes of methods that return boolean",
new String[] {"is", "has", "can", "have", "will", "should"}, 1.0f, ',');
public MethodTypeAndNameIsInconsistentRule() {
definePropertyDescriptor(BOOLEAN_PREFIXES);
}

Then later on, you can use it like this:

Set<String> prefixes = new HashSet<>(Arrays.asList(getProperty(BOOLEAN_PREFIXES)));

Let me know if you have any questions!
Regards,
Andreas


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #109 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AVIDVALCGWGQvxvjWPFPaeTFCeVKXYHAks5qtpmygaJpZM4J63QE.

@adangel

This comment has been minimized.

Member

adangel commented Sep 25, 2016

I think the existing android rules
http://pmd.github.io/pmd-5.5.1/pmd-java/rules/java/android.html are
everything, that is currently existing as android specific rules.
I'd be more than happy to extend these :)

Am 25.09.2016 um 18:35 schrieb ardaasln:

Hello Andreas,

I will work on this and I have an additional question. As PMD website
declares, there are 5 Android specific anti patterns. I am also thinking
to extend it.There are many other android code smells where I think some
of them can be implemented via PMD rules Does PMD work on any other
Android specific anti patterns, if so, what are them.

On 25 Sep 2016, at 18:56, Andreas Dangel notifications@github.com wrote:

Hi @ardaasln https://github.com/ardaasln !

Thanks for contributing!
I think, the rules will be useful. There is only one similar rule existing yet - BooleanGetMethodName http://pmd.github.io/pmd-5.5.1/pmd-java/rules/java/naming.html#BooleanGetMethodName - but it only covers boolean (and only the get/is case).

Here are a couple of notes:

Please make sure, that the code compiles. Currently it has some compilation errors, probably caused the "wrong" quotes in naming.xml https://github.com/pmd/pmd/pull/109/files#diff-a39ce976d0820cebc9d6410a93e7e513R600, and NamingRulesTest.java https://github.com/pmd/pmd/pull/109/files#diff-71e7c9e6dc1757d955edfcccb0dd46c8R34.

I would try to make the examples in naming.xml more concise. It's good to have the complete code in the test, but I think, it's not necessary to write out each case for the example documentation. I think, the following would be enough for MethodTypeAndNameIsInconsistent:

int isotherm() { ... } // ignored - doesn't match Java Beans getter name convention
// the following methods are examples of violations - they all should return boolean
int isValid() { ... }
int hasChild() { ... }
int haveLegs() { ... }
int canFly() { ... }
int willFly() { ... }
int shouldFly() { ... }
// setters shouldn't return anything
int setName(...) { ... }
// getters must return
void getName() { ... }

Similar for AttributeTypeAndNameIsInconsistent:

// the following fields are examples of violations - they all should be of type boolean
int isValid;
int hasMoney;
int canFly;
int willMove;
int haveLegs
int shouldClimb;

It's then enough to mention in the description that both instance variables and local variables are checked.

If we make the prefixes configurable via properties, then even less examples would be necessary.

Maybe I would rename AttributeTypeAndNameIsInconsistent to FieldTypeAndNameIsInconsistent or VariableType... It also seems, that the rule currently only highlights misnamed boolean variables. Maybe its name should even be "BooleanVariableNameIsInconsistent" in it? That would restrict the rule of course to boolean...

Making the prefixes for the boolean cases configurable via parameters makes perfect sense. E.g.

private StringMultiProperty BOOLEAN_PREFIXES = new StringMultiProperty("booleanPrefixes", "the prefixes of methods that return boolean",
new String[] {"is", "has", "can", "have", "will", "should"}, 1.0f, ',');
public MethodTypeAndNameIsInconsistentRule() {
definePropertyDescriptor(BOOLEAN_PREFIXES);
}

Then later on, you can use it like this:

Set<String> prefixes = new HashSet<>(Arrays.asList(getProperty(BOOLEAN_PREFIXES)));

Let me know if you have any questions!
Regards,
Andreas


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #109 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AVIDVALCGWGQvxvjWPFPaeTFCeVKXYHAks5qtpmygaJpZM4J63QE.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#109 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABgDNJBsBNjYDhW7gYYBmEU2-IHtyVEKks5qtqLOgaJpZM4J63QE.

Andreas Dangel
https://github.com/adangel
skype: andreas_dangel

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Nov 29, 2016

@ardaasln are you still interested on working on this? I'd really love to see this included in PMD 5.5.3 and PMD 5.6.0.

@jsotuyod jsotuyod added the a:new-rule label Nov 29, 2016

@ardaasln

This comment has been minimized.

Contributor

ardaasln commented Nov 29, 2016

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Nov 29, 2016

@ardaasln awesome! don't worry, there is no hurry. Just wanted to know if I should pick it up or not.

@jsotuyod jsotuyod added the is:WIP label Dec 2, 2016

@adangel adangel self-assigned this Mar 12, 2018

@adangel adangel changed the title from Add two linguistics rules under naming to [java] Add two linguistics rules under naming Mar 12, 2018

adangel added a commit to adangel/pmd that referenced this pull request Jul 21, 2018

@adangel

This comment has been minimized.

Member

adangel commented Jul 21, 2018

I'm closing this in favor of #1252

@adangel adangel closed this Jul 21, 2018

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