The parameter 'foo' should not be assigned -- what's the harm?

Question

Compare this method:

void doStuff(String val) {
    if (val == null) {
        val = DEFAULT_VALUE;
    }

    // lots of complex processing on val
}

... to this method:

void doStuff(String origVal) {
    String val = origVal;
    if (val == null) {
        val = DEFAULT_VALUE;
    }

    // lots of complex processing on val
}

For the former method, Eclipse emits the warning "The parameter 'val' should not be assigned". Why?

To my eye, the former is cleaner. For one thing, it doesn't force me to come up with two good names for val (coming up with one good one is hard enough).

(Note: Assume there is no field named val in the enclosing class.)

Solution 1

It doesn't look like anyone's made the curmudgeon's case here.

I would generally not mutate parameters, and in fact I tend to mark my parameters final in order to explicitly forbid it. A few reasons:

• Assignment to a parameter could be confused with an attempt to use it as an "output parameter", ref: javapractices.com, and clarity is everything

• Favor Immutability, and that goes for parameter values as much as anything else. Primitives are just a degenerate case of the same thing, it's (generally) easier to reason about immutable variables. Ref, Effective Java Item 13, or javapractices.com

• And finally (NPI), Use final liberally, javapractices.com. However ugly it may be in parameter signatures, I believe it tends to identify unexpected errors and it highlights mutable variables, which generally should be the exception. Most mutable variables in most code are there either for laziness or a perception that it has some effect on performance, when judiciously chosen, immutable, and well-named intermediate computations are clearer, easier to read and verify, and can be cleanly optimized for performance with no help from you.

I can't speak intelligently to your specific case in the abstract, but barring all the other things I might do differently, I'd favor:

void doStuff(final String origVal)
{
    final String valOrDefault = (origVal == null) ? DEFAULT_VALUE : origVal;
    //lots of complex processing on valOrDefault 
}

or even (assuming you wouldn't cope with a null value in a real method with only one argument, it must be part of something more complex)... Also, in general, methods which accept null as a parameter should be explicitly documented as doing so, if only to reinforce the assumption that null parameters ought to be the exception. In the second method, you might even make use of the @NonNull annotation.

/**
  * @param origVal string giving value, possibly null, in which case DEFAULT_VALUE is assigned
  */
void doStuff(final String origVal, ... )
{
    final String valOrDefault = (origVal == null) ? DEFAULT_VALUE : origVal; 
    // similar mucking about to make all the parameters behave, separate from
    // actually operating on them...
    ...
    reallyDoStuff(valOrDefault,...);
}

private void reallyDoStuff(final String value, ...)
{
   assert (value != null);
   // do your complex processing
}

Related questions (and associated argument) on StackOverflow: "Using final modifier whenever applicable in Java...", "final keyword in method parameters", "Do you final-ize local variables and method parameters in Java".

Solution 2

It's sometimes considered a bad practice to reassign parameters inside method. It, probably, comes from C/C++, where calling doSomething(myVar) can change myVar after method is completed. But that's not the case for Java.

IMHO, if you do it as first thing in the method, this is perfectly fine. Everybody reading your code will understand what's going on. It can be confusing if buried deep in the code, though.

Solution 3

In my experience, the use of null as a sentinel for the default parameter is more of an idiom in Python. In Java, you can just overload the method.

void doStuff() {
    doStuff(DEFAULT_VALUE);
}

void doStuff(final String val) {
    assert (val != null); // or whatever
    ...
}

Solution 4

There is a compiler preference which dictates whether or not an instance of parameter assignment is ignored, flagged with a warning, or flagged with an error.

Go to the menu bar - choose Window..Preferences, then In the tree control of the Preferences dialog, select Java..Compiler..Errors/Warnings, then Look in the Code Style section for the "Parameter Assignment" setting.

Solution 5

I suspect it's a style issue; more of a guideline for programmers than an actual potential problem. Some might find it misleading to dispose of the original parameter value.

View more solutions

Comments

[Sebastian vom Meer]

I don't understand what final and output parameters have to do with each other. A final parameter is still mutable and can be used as output parameter. On the other hand final prohibits to assign a new objects to the parameter and makes sure that there is a reference to the original object, so it can be used for output. I think final does not emphasize that a parameter is not an output parameter. In fact, the opposite is the case. Am I wrong?

[andersoj]

@SebastianG If the parameter is a primitive, then final prevents assigning a new value (and thus confusing it with an inout). If the parameter is a reference, then final prevents reassigning it. The parameter itself is therefore not an inout (java passes references by value anyway - stackoverflow.com/questions/40480/is-java-pass-by-reference) but of course you can mutate any objects passed in and use those side-effects for output. The final keyword has no bearing on that.

[Daniele Torino]

My 2 cents: There are no out parameters in java! Anyone who is confused by it should go back to school.Since there are no default parameters in java I check for null and if it's the case I'll assign the default value to the parameter itself. (I know that you can overload methods but there are cases where you need this - e.g. spring mvc)

[SantiBailors]

The question was what's the harm in assigning a value to a parameter.