Always avoid input parameters in Java?
There is no doubt that in-out parameters lead to confusing code as they can increase unexpected / unpredictable side effects.
So, many good programmers say:
Avoid props to change the parameters of the method being modified. Prefer to keep the parameters unchanged.
For the perfectionist programmer who expects his code to be the cleanest and most understandable, should this rule apply in all cases?
For example, suppose the basic method for adding items to a simple list has two paths:
First way (with in-out parameter):
private void addElementsToExistingList(List<String> myList){
myList.add("Foo");
myList.add("Bar");
}
and the caller:
List<String> myList = new ArrayList<String>();
//.......Several Instructions (or not) .....
addElementsToExistingList(myList);
Second way without parameter:
private List<String> addElementsToExistingList(List<String> originalList){
List<String> filledList = new ArrayList<String>(originalList); //add existing elements
filledList.add("Foo");
filledList.add("Bar");
return filledList;
}
and the caller:
List<String> myList = new ArrayList<String>();
//.......Several Instructions (or not) .....
myList.addAll(addElementsToExistingList(myList));
Advantages of the second way:
Parameter unchanged => no risk of unexpected side effects for the new code reader.
Cons of the second way:
Very verbose and very less readable ...
Of course, you would tell me that for code as simple as this, the first way is actually more convenient.
But, if we do not consider the difficulties of any concept / code, I make the second way more logical and obvious to any readers (beginners or not).
However, this violates the CQS principle, which considers "command" methods that have a void return with potential (but allowed by its consent) side effects and "requests" methods that have a return type and no side effects.
So what should a motivating programmer do? Mixing two related code? Or keep the "law" expecting it to always avoid incoming parameters ...
(Of course, the method for adding Element is named to explain the example and would be a misnomer in real code.)
source to share
I think the law should be:
Use what is more straightforward, but always, always detail the behavior of your methods.
The second example is a very good case where, without documentation, you will have a guaranteed error: the method name addElementsToExistingList
, but the method does not add items to the existing list - it creates a new one. A counter-intuitive and misleading name, to say the least ...
source to share
There is a third way. Wrap List<String>
in a class that knows how to add elements to itself:
class ElementList {
private List<String> = new ArrayList<String>();
public void addElements(Element... elements);
}
I like this approach because it keeps the implementation List
private. You don't need to worry if someone passes an immutable list to your method, or if parameters change. The code is simpler. Long method names such as addElementsToExistingList
are code smells that an object is trying to do to something else.
source to share
You should always document when you mutate an object that is a parameter, because otherwise it could have unintended side effects for the caller. In the first case, I agree with others who have commented that the method name is sufficient documentation.
In the second example, elements that are already present in myList appear to have been added twice. In fact, you could completely remove the addElementsToExistingList method parameter and rewrite it as:
private List<String> getElements() {
List<String> filledList = new ArrayList<String>();
filledList.add("Foo");
filledList.add("Bar");
return filledList;
}
List<String> myList = new ArrayList<String>();
//.......Several Instructions (or not) .....
myList.addAll(getElements());
Note that this code is not equivalent to your second example, because the items are only added once, but I think this is actually what you intended. This is the style that I usually prefer. This code is easier to understand and more flexible than the first example, without adding additional code (this can hurt performance a lot, but this is usually not a concern). The getElements () client can now also do other things with a list of elements besides adding it to an existing collection.
source to share
In the case of your example, the name makes it clear - "addElementsToExistingList" seems pretty clear to me to hint that you're going to ... uh ... you know. But your concern would be justified with a less obvious name.
For example in ruby this is usually handled by naming conventions
"a".upcase
=> gives an uppercase variable, leaves the original unchanged
"a".upcase!
=> modifies the original variable
source to share
It's fine to change / mutate parameters as long as they are documented. And, of course, with the method name "addElementsToExistingList", what else would anyone expect? However, as stated earlier, your second implementation returns a copy and does not modify the original, so the method name is now misleading. Your first path is a perfectly acceptable way of doing things. The only additional improvement is the addition of a true / false value in return indicating true if only all items have been added to the list.
source to share