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.)

Answer 1

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.

Answer 2

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.

Answer 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
    ...
}

Answer 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.

Answer 5

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".