Skip to main content

Don't miss this

Posted by malenkov on April 13, 2009 at 5:00 AM PDT

I would like to discuss code conventions. In particular, the usage of the this keyword.

Recently we have argued with one of my colleagues again. I always use the this.name pattern to access class fields. Whereas he says that I should not differ from everybody else and use this to avoid shadowing only. Let me explain my position on the question.

The this keyword is commonly used to access the class fields, which are shadowed by a method or constructor parameter. For example,

public class Bean {
     private int value;
     public void setValue(int value) {
         this.value = value;
     }
     public int getValue() {
         return this.value;
     }
}

In the setter, you need to use the this keyword because of the shadowing unless you are going to look for synonyms to name parameters. In the getter, the this keyword can be omitted, but I never do so due to the following reasons.

First, one of the reasons for the creation of the Code Conventions for the Java Programming Language document was an improvement of code readability. I think that the this.name identifier bears more clarity than just name does. If you follow such pattern, a code maintainer will quickly detect whether it is a class field or a local variable used.

Second, I do not like conditional rules, when the way you implement a standard action depends on some other conditions. Instead, I would rather prefer consistency. If class fields are accessed by using the this keyword in one case, it is reasonable to access class fields by using the same pattern in the rest of the situations.

Third, the this keyword allows developers to avoid hard-to-detect errors when they modify a method with a huge code and introduce a variable that shadows a class field. For example, let's declare the directory variable in the following method that is huge. The rest of the method will use this local variable instead of using a class field. As a result, the code works differently. There are no compiler-time errors, but the method results are unexpected at run-time.

class Location {
     private File directory;
     public void process(String mask) {
        ...
         Object object1 = directory;
         ...
         File directory = null;
         ...
         Object object2 = directory;
         ...
     }
}

Finally, I have to read a code using not only IntelliJ IDEA but some other tools as well. Quite often I need to review a code sent by my colleagues as either an HTML or a diff-file. In such cases it is rather difficult to detect the scope of the identifier if the code is huge. Look at the addImpl method of the Container class. How fast will you determine what the component identifier mean? How fast will you determine that the component identifier does not correspond to the comp method's parameter?

protected void addImpl(Component comp, Object constraints, int index) {
    synchronized (getTreeLock()) {
        /* Check for correct arguments:  index in bounds,
         * comp cannot be one of this container's parents,
         * and comp cannot be a window.
         * comp and container must be on the same GraphicsDevice.
         * if comp is container, all sub-components must be on
         * same GraphicsDevice.
         */
        GraphicsConfiguration thisGC = this.getGraphicsConfiguration();

        if (index > component.size() || (index < 0 && index != -1)) {
            throw new IllegalArgumentException("illegal component position");
        }
        checkAddToSelf(comp);
        checkNotAWindow(comp);
        if (thisGC != null) {
            comp.checkGD(thisGC.getDevice().getIDstring());
        }

        /* Reparent the component and tidy up the tree's state. */
        if (comp.parent != null) {
            comp.parent.remove(comp);
            if (index > component.size()) {
                throw new IllegalArgumentException("illegal component position");
            }
        }

        //index == -1 means add to the end.
        if (index == -1) {
            component.add(comp);
        } else {
            component.add(index, comp);
        }
        comp.parent = this;

        adjustListeningChildren(AWTEvent.HIERARCHY_EVENT_MASK,
            comp.numListening(AWTEvent.HIERARCHY_EVENT_MASK));
        adjustListeningChildren(AWTEvent.HIERARCHY_BOUNDS_EVENT_MASK,
            comp.numListening(AWTEvent.HIERARCHY_BOUNDS_EVENT_MASK));
        adjustDescendants(comp.countHierarchyMembers());

        invalidateIfValid();
        if (peer != null) {
            comp.addNotify();
        }

        /* Notify the layout manager of the added component. */
        if (layoutMgr != null) {
            if (layoutMgr instanceof LayoutManager2) {
                ((LayoutManager2)layoutMgr).addLayoutComponent(comp, constraints);
            } else if (constraints instanceof String) {
                layoutMgr.addLayoutComponent((String)constraints, comp);
            }
        }
        if (containerListener != null ||
            (eventMask & AWTEvent.CONTAINER_EVENT_MASK) != 0 ||
            Toolkit.enabledOnToolkit(AWTEvent.CONTAINER_EVENT_MASK)) {
            ContainerEvent e = new ContainerEvent(this,
                                 ContainerEvent.COMPONENT_ADDED,
                                 comp);
            dispatchEvent(e);
        }

        comp.createHierarchyEvents(HierarchyEvent.HIERARCHY_CHANGED, comp,
                                   this, HierarchyEvent.PARENT_CHANGED,
                                   Toolkit.enabledOnToolkit(AWTEvent.HIERARCHY_EVENT_MASK));
        if (peer != null && layoutMgr == null && isVisible()) {
            updateCursorImmediately();
        }
    }
}
Related Topics >>

Comments

At the company I am working I have seen this: private int m_count; public void getM_count(){ return m_count; } unbelievable!!!

@opinali "But the dominant opinion is most often right..." Interesting that you mention that exceptions to your rule are the innovations. As I see, most often the confrontation of opinions involve an innovation that opposes to the mainstream. And the human kind tends to be reactionary, i. e., does not like changes (inertia). Of course, sometimes the dominant opinion changes. Sometimes, because the new opinion is the best one, sometimes because a more powerful institution has more money to make more lobby (which is definitely not the case, here). But due to the resistance of changes of the human kind, the advantage of the new opinion must be considerably bigger than the dominant's opinion, in order to provide "activation energy" for the change. So I can't agree that the dominant opinion is most often right. But less about philosophy, more about coding style. I doubt that the use of "this" is compelling enough to become mainstream, but I like it very much. First of all, I don't see the "this." as a prefix, but as a namespace, like java packages. Yes, there is no namespace for the local variables, but if there was, I would use it. As a matter of fact, I often type "this." in order to have eclipe to list the class properties for me, and auto-complete it. Second, by using "this." as a rule, and not as an exception, I consider less probable to shadow a class variable by mistake. But unlike malenkov, I think that rules don't have to be unbreakable. I believe rules are good, but a good developer should always know when to break them. As an example, I have never seen a good set of rules to split code in many lines that never leads to an unreasonable, less readable code, in some (not rare) cases. So, in a very long equation being calculated, I would consider skipping the "this." from the code. But I have never worked in a program in Java with so many calculations using class properties. So, in my perspective, this argument would mean to have the tail wagging the dog.

@malenkov: Working with code in email sucks indeed, I must do that sometimes, and I'd love to find a Thunderbird add-on that detects source attachments and highlights them (multiple open source highlighting engines could perhaps be reused). What's the problem of just extracting these attachs on top of your checkout of the project, then using your IDE's diff viewer to compare that code to the repo? You don't even need a clean checkout if your IDE supports a Local History (both Eclipse and recent versions of NetBeans support this). Some IDE / email client integration could also help (yeah I know that emacs can probably do that, I'm just not smart enough to use emacs). Now if the patch is very big (many files or very large/complex diffs), I agree that you don't want to overwrite your main workspace because rolling back can be difficult unless you started from a clean workspace; in that case, the best choice is really a clone or dedicated checkout.. but you are probably going to spend hours reviewing that huge patch, so the work/time needed for a new checkout or clone is probably irrelevant... you may even be forced to review on a full blown development environment because the review requires to run unit tests or observe the changed code's dynamics in a debugger. If you spend so much of your workflow reviewing email patches, I have two thoughts. First, reading those emails is hopelessly bad even with 'this.' and other tricks, because you can't mouse-over a field reference to see its declaration or Javadoc, or double-click a method call to jump to that method's code, or click some identifier to automatically highlight all its uses in the class..., among other "killer features" that modern IDEs offer for code reviewing. You are screwed anyway, your 'this.' will only solve 1% of the problem - and make all code worse for team members that DON'T need to review email patches - so the only solution I can see is my suggestion above. And back to my example.. once again, static variables are very clearly distinguished by syntax highlight. And I suppose you would also prefix all uses of static vars with the class name because you might have a method that receives a parameter with the same name of some static field? P.S.: I know that you derived your habits from long experience, so it's pointless to debate what's best for you - it's not my intention. Perhaps you have very specific problems or strong feelings about code aesthetics and other issues (just like I have my own). I'm just trying to put the whole debate on objective terms - I suppose that was also your intention, e.g. convincing the world, though your blog, that your code style "differs from everybody else" not due to subjective feelings but because of some rock-solid reasoning. But the dominant opinion is most often right, precisely because it's the objective advantages (Darwinism) that usually make that opinion dominate. ;-) Most exceptions to that rule are those caused by innovations, e.g. if advanced IDEs were a fresh novelty we could be using many style traits that would be dominant but not anymore objectively superior. But this doesn't seem to apply to the 'this.' problem.

@opinali: Unfortunately, I should review the code that colleagues send me via email. Do you suggest to clone workspace, patch it and load into IDE? How to force my colleagues to send highlighted diff-file? About your example. The second case is better because it says that you use instance variables to calculate the area. The first one shows the formula, but several fields can be static. Consider the following example from java.awt.Button: String constructComponentName() { synchronized (Button.class) { return base + nameCounter++; } } You can't guess that this method uses static variables, because it is a class member.

@malenkov: - Good tools give you full syntax highlight not only in full-blown editors but also in diff visualization and file comparing/merging, and anywhere a project file is exhibited in full or in part. Now I agree that you may have to use some tool that doesn't have syntax highlight, but I suppose this is not a major part of your workflow, right? How much time do you spend (say) reviewing changesets in Mercurial's crappy web-based diff viewer, relative to the time you work with code with adequate tools? Choosing your code conventions based in the weaker and less frequently used tools is clearly a case of the tail wagging the dog. - I also use static import carefully, usually just for things like Math.*; wildcard package imports are also prohibited in my Checkstyle settings. But that was just an example, because it's a similar situation, i.e. if I write "abs(x)" instead of "Math.abs(x))" it's less explicit where abs() comes from. - I also follow the rule that fields are always private. And this rule makes my argument stronger because, besides encapsulation, it has the side benefit of reducing identifier pollution (see my reply to philho). Even in the leaf of a 20-class-high hierarchy where I'm inheriting a hundred of fields (say a Swing component), I can only "see" the half-dozen fields of the current class + the locals of each method, which means that I have so few variables to care about that conventions like 'this.' to distinguish fields from locals are pointless. Proper code structuring relieves you from verbosity like 'this.' or 'm_'. - The 'this.' prefix is actually a very limited form of Hungarian notation (HN is not used only for typing - yeah I'm also a Petzoldian in a previous life, wrote my share of HN.) And long identifiers decrease readability, I agree, but how exactly is a five-character prefix in EVERY field use make my identifiers less long? Notice that the problem is not identifier length, but the overall code complexity (more stuff to read, more tokens, characters, anything). The prefix can make a very readable code like this... double area () { return (xEnd - xStart) * (yEnd - yStart); } ...into something twice as big, and clearly less readable like this: double area () { return (this.xEnd - this.xStart) * (this.yEnd - this.yStart); } ...and notice that I didn't even pick a really bad example, like a method with very complex expressions involving a large number of fields, each with single-letter names. And yes you can have lots of such code in real-world projects (e.g. in your paint() method with complex J2D code, or in some Billing application with complex financial maths, etc.). When confronted with situations like above, I guess you end up with workarounds like breaking expresions into more lines (which is even less readable compared to the original), or making a special case to not use 'this.' (breaking your rule of no special cases); or you just live with code that's indisputably much bigger and harder to read?

@philho: The 'm_' prefix is illegal because field names must use camelCase style without underscores; underscores are only permitted in CONSTANT_NAMES. This is not a JLS rule but it's among the most fundamental laws of Java style. Anyway, this is indeed a habit carried from other languages like C/C++ that suffer from additional problems like the existence of tons of identifiers in the global scope (including variables, functions, macros, compiler predefs, etc.) so programmers often have more pressure to make scope explicit in identifier names. Additionally, in C/C+_ it is common to have structs or classes with a stupid number of fields (dozens and more), and functions with several pages and also dozens of local variables and parameters, so the identifier pollution is often enormous, again an excuse for things like 'm_'. The solution is not using such conventions, the solution is using proper OO design, refactoring, etc. With Java I use Checkstyle to impose upper limits on max number of parameters per methods, fields per class, lines per method etc., and I very rarely hit those limits; so the identifier pollution is tiny and at any point in the code there's only a relatively small number of variables in scope, so I don't have to trash the code with 'this.', 'm_' and the likes.

@philho: I don't like underscore. I think that identifiers should have only letters (sometimes digits at the end). The this keyword makes code similar to English. For example, this value is equal to that value: class MyObject { private int value; boolean equals(MyObject that) { return this.value == that.value; } }

@opinali: - I'm talking about Java. I don't want to discuss scripting languages. I use Java5 static import very careful in small files only. And I never import all classes and members (never import package.*) - I don't use "this" to call methods. You can't declare some method within another one. Also I think that class fields should be private always. - Sometimes "modern IDEs/editors" are not available. For example, look at the example in the blog post. My arguments are from real life. Hungarian Notation is funny, but I don't need to know the field type in every line. Also long identifiers decreases readability.

@samkass: I agree with you and prefer strict code conventions per project. Now I'm trying to convince our project leader. There are 3 programmers on the project and we use diff-files to code review (see my last reason). It is a startup project and we can choose code style before we create a lot of files.

I disagree with opinali, I prefer my "dumb" editor to Eclipse because the latter is sooo slow sometime, we post code in Web pages and syntax highlighters are absent or "dumb", we use the terminal and diff utilities, etc. Now, I also disagree with this usage of 'this', which I use only to get a reference to the class (pass to constructors, etc.). I use a not so uncommon convention, coming perhaps from Microsoft coding practices, to prefix field variables with m_ (and static variables with s_). Lot of Java purists will puke at that, and indeed it is only marginally better that 'this.' prefix (slightly shorter...) but well, I like it... :-) Funnily, that's also part of coding conventions we use at work (coincidence, but the fact the lead programmer has Windows programming background might not be foreign to this...). Other people just prefix (or suffix) parameters with underscore (ie. _value or value_).

All your arguments are (IMO) much weaker than those against unnecessary use of 'this.': - Avoiding redundant code / boilerplate; the trend in programming language is towards less junk, not more. I guess you also oppose to enhancements like type inference (like in Scala, JavaFX, and virtually all modern static-typed language designs)? Or even Java5 static import? - In block-structured languages, it's a long tradition to consider scope as parte of the context that helps understand code. I suppose you also prefix with 'this.' all calls to methods from the same object? - With modern IDEs/editors you can highlight attributes differently than local variables so there's zero chance of mistakes of readability issues. Your argument of having to read diff files and such without proper tools is a clear strawman - the same argument would stand behind other horrible practices, e.g. let's use Hungarian Notation because type declarations are often missing in diffs when the declaration line was not touched. Even exceptional rules are often good for exceptional conditions. I use 'this.' in constructors and setters, where I name parameters like the attributes they initialize (this.value = value), because such initialization is a very particular and important case (@see also JavaFX Script's property design). But if you don't like this style, just use different names like newValue instead of value, etc.

Using 'this' like you helps me identify my code better as well which for me, is the main thing. http://www.castlesteps.com

Absolutely agree with you, Sergey, for all the reasons you list.

The third argument is the most important. It helps to avoid stupid errors such as public void setValue(double value) { value = value; } Not much a concern with the modern IDEs anymore, but I remember seeing this even in JDK.

I agree with you Sergey, and I also prefer using this to mark instance member fields in my code. This definitely improves readability and reduces how much "scanning" one must do to identify what type of variable you're dealing with, especially if you're not using an IDE.

"Whereas he says that I should not differ from everybody else..." This is an extremely compelling argument. (I'd say more compelling than any of the arguments you put forward.) Everyone has their own style, but on a project unless everyone follows the common standard the code readability drops dramatically. If you can convince everyone else to adopt your "this" syntax, then go for it. Otherwise, I'd say conform.