Should RequireThis validation be included in Checkstyle?

Question

One of Checkstyle's built-in checks is RequireThis , which will be disabled if you do not add this. to a local call to fields or methods. For example,

 public final class ExampleClass { public String getMeSomething() { return "Something"; } public String getMeSomethingElse() { //will violate Checkstyle; should be this.getMeSomething() return getMeSomething() + " else"; } } 

I am afraid if this check is justified. In the above example, ExampleClass is final, which should ensure that the "correct" version of getMeSomething . Also, there seem to be cases where you might want subclasses to override the default behavior, in which case the “this” requirement is the wrong behavior.

Finally, it looks like an overly protective coding behavior, which only clutters the source and makes it difficult to see what is actually happening.

So, before I suggest to my architect that this is a bad check to enable, I wonder if anyone else has included this check ? Are you in critical error due to the lack of this ?

Answer 1

The RequireThis rule has the correct use, since it can prevent a possible error in the methods and constructors when it is applied to fields. The code below is almost certainly an error:

 void setSomething(String something) { something = something; } 

Code like this will compile, but will do nothing but override the value of the method parameter to itself. Most likely, the author intended to do this:

 void setSomething(String something) { this.something = something; } 

This is a typo that can happen and deserves to be checked, because it can help prevent debugging problems if the code failed because this.something not installed much later in the program.

The checkstyle settings allow you to save this useful check for fields, at the same time omitting the largely unnecessary method check, setting up the rule as follows:

  <module name="RequireThis"> <property name="checkMethods" value="false"/> </module> 

When it comes to methods, this rule has no real effect, because calling this.getMeSomething() or just getMeSomething() does not affect the resolution of the Java method. The call to this.getSomethingStatic() still works when the method is static, it is not an error, it is only a warning in various IDEs and static analysis.

Answer 2

Challenge with that. does not stop the call from invoking the overridden method in the subclass, since it refers to "this object" and not to "this class". This should prevent you from mistakenly accepting a static method for an instance method.

To be honest, this does not seem to be a particularly common problem, I personally do not think it was worth the compromise.

Answer 3

Personally, I would not turn it on. Mostly because whenever I read the code, I read it in the IDE (or something else that the smart code formatting does). This means that various method calls and field calls are formatted based on their actual semantic meaning and are not based on some (possibly incorrect) indication.

this. not required for the compiler, and when the ID environment does smart formatting, this is not necessary for the user. And writing unnecessary code is just the source of errors in this code (in this example: using this. In some places and not using it in other places).

Answer 4

I would definitely disable it. Using this.foo() is non-idiomatic Java and should therefore only be used when necessary to signal that something special is happening in the code. For example, in the setter:

  void setFoo (int foo) {this.foo = foo;}

When I read code that makes it useless, I usually mark it as a programmer without a clear understanding of object-oriented programming. Largely because I usually saw this style of code with programmers who do not understand that this is not required everywhere.

I am sincerely surprised to see this, as a rule, in the CheckStyle library.

Answer 5

I would enable this check only for fields, because I like the additional information added by ' this. 'in front of the field.
See My (old) question: Are you prefixing your instance variable 'this in java? .

But for any other project, especially for an obsolete one, I would not activate it:

• Most likely the keyword is' this. 'almost never used, which means that this check will generate a ton of warnings.
• redefinition of names (for example, a field and a method with a similar name) is very rare due to the fact that the current IDE flag has a warning code of its own by default.