Code Advice #11: Initialize fields using the property name

(See intro for a background and caveats on these coding advice blog entries.)

How do you initialize fields in a constructor? And how do you initialize fields in a JavaBean setter method?

Here's the, in my opinion, correct way to do it:

    void setTemperature(int temperature) {
        this.temperature = temperature;
    }
The same scheme is used in constructors.

There are various other ways to do this. Dave Yost argues that you should use the following pattern:

    void setTemperature(int newTemperature) {
        temperature = newTemperature;
    }
Another coding style guide recommends that you "consider using the prefix a or an with parameter names. This helps make the parameter distinguishable from local and instance variables":
    void setTemperature(int aTemperature) {
        temperature = aTemperature;
    }
A third scheme I've seen quite frequently is choosing a parameter name that is an abbreviation for the property name being set:
    void setTemperature(int temp) {
        temperature = temp;
    }
A fourth suggestion is to use underscores as suffixes on the field names themselves:
    void setTemperature(int temperature) {
        temperature_ = temperature;
    }

So, why do I think my way is the right way to do it, and not the other alternatives shown? As always, the first reason is that the this.x=x pattern is the most common way to do it - and as a consequence more developers will find your code clear and pleasant to read.

However, there are logical reasons to do it this way too. The primary reason is that the signature of your method is part of its API! If this is a public method, the parameter should be included in the javadoc with a @param tag. With the first three alternatives, the parameter names are newTemperature, aTemperature and temp. I think temperature is a better parameter description. Yes, it's true that for a setter method like setTemperature, it's not that important what the parameter name is since it's obvious what it's for. But when you're dealing with a constructor, you do need to be descriptive. And newTemperature is not a proper name for an initial state. You could switch to using initial instead of new as a prefix, but in addition to being wordy you now have different schemes for constructors and setters. Having one approach to both is beneficial since it keeps things simple.

The last alternative does let you use a good parameter name - the same one that I'm proposing, temperature. However, it has another disadvantage in that it's using an underscore_ as a suffix for the field name. This to me smacks of Hungarian notation, although rather than describing the object type with a prefix it describes the symbol class with a suffix. I don't like it since it makes the code less readable, but I suppose that's a topic for a whole other discussion. In short though, most Java programmers avoid underscores in all symbols but constants (such as public static final int WATER_BOILING = 100;).

The proposed style does however have one problem. It relies on "polluting" the namespace in the constructor or setter with a parameter which hides the field name. In the following code, temperature refers to the parameter, not the field:

    void setTemperature(int temperature) {
        this.temperature = temperature;
    }
What happens if you accidentally write the wrong parameter name - either by a typo, or by writing a similar name and not noticing that the parameter name is a different variation of the field name?
    void setTemperature(int temp) {
        this.temperature = temperature;
    }
The above code will compile just fine. However, it is probably not what is intended. If you call this method, the temperature field will not be updated - you are simply assigning the current value of the field to itself.

Luckily, your programming tools will find these errors for you. Obviously, a tool could warn you that the temp parameter is unused, and that would clue you in. But most people do not want warnings on unused parameters, since they occur frequently in a lot of code - especially when you implement interfaces.

However, the code above has a "self assignment" - the left hand side of the assignment is identical to the right hand side. This code is obviously redundant and can be removed - but more importantly, it usually points to an error of the above form. Therefore, if you run findbugs (NetBeans plugin here), it will clue you right in to the problem:

Jackpot can find self assignments as well. The following code is courtesy of Tom Ball and will be part of the Examples shipped with Jackpot (and hopefully be built in as well).

import org.netbeans.jackpot.query.Query;
import javax.lang.model.element.Element;
import com.sun.source.tree.AssignmentTree;

public class SelfAssignmentQuery extends Query {

   public Void visitAssignment(AssignmentTree tree, Object p) {
      Element lhs = model.getElement(tree.getVariable());
      Element rhs = model.getElement(tree.getExpression());
      if (lhs == rhs) { // Elements are unique so == is okay
         addResult(getCurrentElement(), tree, "self-assignment found");
      }
      return super.visitAssignment(tree, p);
   }
} 

Armed with the right tools, the this.x=x pattern works because you get to use the "right" parameter name, and the tools will catch your mistakes.

Comments:

I prefer an even different style. I use the underscore as a prefix, not a suffix. So it would be
   void setTemperature(int temperature) {
      _temperature = temperature;
   }
I find that more readable than temperature_. An advantage of this style is the fact that you can tell by a variable's name whether it's a local or a field.

Posted by Paul Häder on July 25, 2006 at 02:06 AM PDT #

Once again, I totally agree with you Tor. I wouldn't be surprise if we had the same code style.

Posted by Romain Guy on July 25, 2006 at 04:26 AM PDT #

I have to say that I disagree with your style, and blogged my reasons here.

Posted by Oliver Burn on July 25, 2006 at 08:48 AM PDT #

I gave you a Jackpot query that has false positives; because array subscripts and method invocations don't have a unique Element, the above code will incorrectly report a case such as "foo[i] = getFoo();". Here is the corrected method (still short):
   public Void visitAssignment(AssignmentTree tree, Object p) {
      Element lhs = model.getElement(tree.getVariable());
      Element rhs = model.getElement(tree.getExpression());
      if (lhs == rhs) { // Elements are unique so == is okay
         addResult(getCurrentElement(), tree, "self-assignment found");
      }
      return super.visitAssignment(tree, p);
   }

Posted by Tom Ball on July 25, 2006 at 11:59 PM PDT #

I meant the following code:
    public Void visitAssignment(AssignmentTree tree, Object p) {
        Element lhs = model.getElement(tree.getVariable());
        if (lhs != null) { // true for method invocations and array elements
            Element rhs = model.getElement(tree.getExpression());
            if (lhs == rhs) {
                String msg = NbBundle.getMessage(FindSelfAssignments.class, "FindSelfAssignments.found");
                addResult(getCurrentElement(), tree, msg);
            }
        }
        return super.visitAssignment(tree, p);
    }
The issue is that model.getElement() returns null for trees that don't have associated elements, and in the old code "null == null" is a match.

Posted by Tom Ball on July 26, 2006 at 12:02 AM PDT #

Whilst not so much a stylistic convention you can use the java final keyword to help prevent self assignment e.g.
public class Foo {
   private final String foo;

   public Foo(final String foo) {
      foo = foo; //compiler won't allow
      
   }
}
Obviously this is not applicable in all cases, as some variables will require reassignment though I find that the majority don't. I tend to try and finalise as many variables as possible in my code, as it foces you to think explicitly about reassignments as a seperate case.

Posted by Rick on July 30, 2006 at 07:50 PM PDT #

It would be nice if NetBeans would indicate in the editor what the scope of identifiers are. E.g. in Eclipse you can specify different colors for data members, parameters, and class members. From what I've seen sofar in NetBeans, this is not possible, although I have to admit that I didn't spend much time on it.

Posted by Frank Kieviet on July 31, 2006 at 10:10 AM PDT #

Hi Frank, this is being worked on for NetBeans 6. Take a look at http://blogs.sun.com/roller/page/jlahoda?entry=new_features_in_netbeans_ide.

Posted by Tor Norbye on July 31, 2006 at 10:31 AM PDT #

Post a Comment:
Comments are closed for this entry.
About

Tor Norbye

Search

Archives
« July 2014
SunMonTueWedThuFriSat
  
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
  
       
Today