Should the RequireThis check in Checkstyle be enabled?

Question

One of the built-in Checkstyle checks is RequireThis, which will go off whenever you don't prepend this. to local field or method invocations. 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'm struggling with whether this check is justified. In the above example, the ExampleClass is final, which should guarantee that the "right" version of getMeSomething should be invoked. Additionally, there seem to be instances where you might want subclasses to override default behavior, in which case requiring "this" is the wrong behavior.

Finally, it seems like overly defensive coding behavior that only clutters up the source and makes it more difficult to see what is actually going on.

So before I suggest to my architect that this is a bad check to enable, I'm wondering if anyone else has enabled this check? Have you caught a critical bug as a result of a missing this?

Answer 1

Calling with "this." does not stop the invocation from calling an overridden method in a subclass, since this refers to "this object" not "this class". It should stop you from mistaking a static method for an instance method though.

To be honest, that doesn't sound like a particularly common problem, I personally wouldn't think it was worth the trade-off.

Answer 2

Personally I wouldn't enable it. Mostly because whenever I read code I read it in an IDE (or something else that does smart code formatting). This means that the different kind of method calls and field accesses are formatted based on their actual semantic meaning and not based on some (possibly wrong) indication.

this. is not necessary for the compiler and when the IDE does smart formatting, then it's not necessary for the user either. And writing unnecessary code is just a source of errors in this code (in this example: using this. in some places and not using it in other places).

Answer 3

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

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

When I read code that makes gratuitous use of this, I generally mark it up to a programmer without a firm grasp on object-oriented programming. Largely because I have generally seen that style of code from programmers that don't understand that this isn't required everywhere.

I'm frankly surprised to see this as a rule in CheckStyle's library.

Answer 4

I would enable this check only for fields, because I like the extra information added by 'this.' in front of a field.
See my (old) question: Do you prefix your instance variable with ‘this’ in java ?.

But for any other project, especially legacy ones, I would not activate it:

• chances are, the keyword 'this.' is almost never used, meaning this check would generate tons of warnings.
• naming overrides (like a field and a method with a similar name) are very rare due to the current IDE flagging by default that code with a warning of their own.

Answer 5

The RequireThis rule does have a valid use in that it can prevent a possible bug in methods and constructors when it applies to fields. The code below is almost certainly a bug:

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

Code like this will compile but will do nothing except reassign the value of the method parameter to itself. It is more likely that the author intended to do this:

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

This is a typo that could happen and is worth checking for as it may help to prevent hard to debug problems if the code fails because this.something is not set much later in the program.

The checkstyle settings allow you to keep this useful check for fields while omitting the largely unnecessary check for methods by configuring the rule like this:

   <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() has no effect on Java's method resolution. Calling this.getSomethingStatic() still works when the method is static this is not an error, it is only a warning in various IDEs and static analysis tools.