Monday, June 14, 2010
My current employer runs Sonar's static code analysis on the continuous integration servers. I recently turned on an IDE plugin which integrates Sonar's results into my editor and noticed a warning that made me scratch my head. Sonar doesn't like that I sometimes reuse my parameter variables. Here's an example of what I mean:
public void doSomething(int input) {
input = Math.min(20, input);
input = Math.max(0, input);
for (int i = 0; i < input; i++) {
doSomethingElse(input);
}
}
I went online to look for an explanation. I expected to find an academic paper explaining some details that I'm overlooking. Instead I found that everyone willing to speak seemed to be in agreement that this is a bad practice. Strangely, I could find little reasoning as to why. The best explanation I could find was Martin Fowler's Refactoring. In it he provides two reasons you shouldn't make assignments to parameters:
1) Java uses pass by value exclusively. "With pass by value, any change to the parameter is not reflected in the calling routine. Those who have used pass by reference will probably find this confusing"
2) Consistency - "It is much clearer if you use only the parameter to represent what has been passed in, because that is a consistent usage."
To the first point, I would say any Java developer (even a junior developer) who thinks reassigning a parameter will effect the reference passed into the method should be fired immediately. We shouldn't cater to that developer. They will eventually do severe damage to your program.
To the second point, I think creating a second variable for the same purpose of the first is the very definition of inconsistent. A parameter is nothing more than a method scoped variable that happened to have been assigned from outside the method. If James Gosling had intended them to be final, he would have made them that way.*
I believe I like re-assignment for the very reason Fowler's dislikes them: maintainability and readability. The result of refactoring the above code to Fowler's recommendation would result in this code:
public void doSomething(int input) {
int boundedInput = Math.min(20, input);
boundedInput = Math.max(0, boundedInput);
for (int i = 0; i < boundedInput; i++) {
doSomethingElse(boundedInput);
}
}
I think this is less readable. We now have two variables of the same scope that have nearly the same meaning. The first (the parameter) should not be used anywhere in the method except to initialize the second variable. This seems error prone. Somone could easily use input in the method body where they meant to operate against boundedInput. Even if they don't, it's still there taking up space (and brain cells) without providing anything to the story of this method.
Maybe I'm alone in thinking reusing parameters can make for more readable code. What better way to find out than for me to post my thoughts for the world to read (and criticize)? Feel free to comment.
* I guess an argument could be made that it was a mistake not to make parameters final by default, but now is a little late to make that argument. I believe we should write in the language that we have.
posted by Bryan at
9:08 PM
|
3 Comments
