Is "this" shadowing a good idea?

Question

The case of shadowing class variables is common in in Java. Eclipse will happily generate this code:

public class TestClass {
    private int value;
    private String test;
    public TestClass(int value, String test) {
        super();
        this.value = value;
        this.test = test;
    }
    public int getValue() {
        return value;
    }
    public void setValue(int value) {
        this.value = value;
    }
    public String getTest() {
        return test;
    }
    public void setTest(String test) {
        this.test = test;
    }
}

Is variable shadowing ever ok?

I am considering the implementation of a coding rule saying that "shadowing will not be allowed." In the simple case above it is clear enough what is going on. Add in a little more code that does something and you run the risk of missing "this" and introducing a bug.

What is the general consensus? Ban shadowing, allow it sometimes, or let it roll?

Answer 1

I actually prefer the guideline "Shadowing is only allowed in constructors and setters". Everything else is not allowed.
Saves you the trouble of naming constructor arguments aValue and aTest just to avoid shadowing.

If you're using eclipse, its warnings settings can be set exactly to that option BTW.

Answer 2

I feel safe with variable shadowing when using IDEs such as Eclipse and IntelliJ IDEA which highlight fields in different colors than the local variables and also provide helpful warnings on local variable mis-uses.

Answer 3

Shadowing can be useful in simple code such as constructors getters setters and anything of the sort.

However the use of descriptive variables is really important so instead of using this

this.name = name; try this this.name = newName;

Also, if you make a habit of including this. in your code it becomes second nature and helps quite a bit with readability

Answer 4

A good IDE like Eclipse shows you, in different colours and/or fonts, the attributes and the method variables of your class. Becaus of that variable shadowing is OK.

Answer 5

I actually set up my install of Eclipse to issue warnings for every under-qualified variable. This ensures I never forget to prefix implementation variables with this.. This has effectively preemptively solved any problem that might arise from shadowing.

You can do this by way of Preferences > Java > Compiler > Errors/Warnings >> Code Style > Unqualified access to instance field.

Answer 6

I do "this" shadowing all the time. In complex places, it's useful to use an explicit this even if it doesn't shadow anything. It makes it easier to distinguish between local and class variables, from the human viewpoint (although, then it becomes an issue that you must be consistent; using this a little bit here and there but not everywhere is confusing).

In Python, you don't even have the choice: plain x is always local. Class members are self.x.

Answer 7

Shadowing is always bad. Name your variables after their scope (not type), and you'll save yourself the hassle.

Answer 8

Well, I dont see any problem with this code. Its good that IDE is helping you to reduce the amount of code that you have to write.

Answer 9

A valid case is that it provides a telling signature, so those maintaining the app Can easily see which fields are passed in the Call.

The Builder pattern is, however a better solution for maintainability.

Answer 10

The main justification for style rules is to make code readable, both to the original author and to others who need to maintain it. In this context, readability is about being able to easily understand what the code actually does, both at the mechanistic level and at the deeper semantic level.

In general, (apart from constructors and setters) variable hiding tends to be considered bad style because it causes the casual reader to mistake uses of locals for uses of members, and vice versa. (An IDE that highlights member names tends to mitigate this, but it is still easy to miss the distinction.) And (apart from constructors and setters) there is generally a clear semantic distinction between the local and the member with the same name, and this is best reflected by using different names.

Setters and constructors are a bit different in each of the respects above. Since setters (in particular) and constructors are simple and stylized, the hiding of the form shown is unlikely to cause a casual reader and confusion. Indeed, I would argue using only one identifier for what is essentially the same information may actually make the code easier to read.

On this basis, I would say that hiding in constructors and setters is perfectly acceptable. A style rule that rigidly insists that you should avoid hiding in this context is (IMO) pedantic and maybe counter-productive. And it is certainly out of step with what most Java coders would consider to be normal practice.