WARNING: This blog entry was imported from my old blog on blogs.sun.com (which used different blogging software), so formatting and links may not be correct.
(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.
