Is modifying an incoming parameter an antipattern?

Question

I am programming in Java, and I always make converters sort of like this:

public OtherObject MyObject2OtherObject(MyObject mo){
    ... Do the conversion
    return otherObject;
}

At the new workplace the pattern is:

public void MyObject2OtherObject(MyObject mo, OtherObject oo){
    ... Do the conversion
}

For me it is a little bit smelly, as I got used to not to change the incoming parameters. Is this incoming parameter alteration an antipattern or is it OK? Does it have some serious drawbacks?

Answer 1

It's not an antipattern, it's a bad practice.

The difference between an antipattern and a mere bad practice is here: anti-pattern definition.

The new workplace style you show is a bad practice, vestigial or pre-OOP times, according to Uncle Bob's Clean Code.

Arguments are most naturally interpreted as inputs to a function.

Anything that forces you to check the function signature is equivalent to a double-take. It’s a cognitive break and should be avoided. In the days before object oriented programming it was sometimes necessary to have output arguments. However, much of the need for output arguments disappears in OO languages

Answer 2

Quoting Robert C. Martin's famous book "Clean Code":

Output arguments should be avoided

Functions should have a small numbers of arguments

The second pattern violates both rules, particularly the "output argument" one. In this sense it is worse than the first pattern.

Answer 3

The second idiom can be faster, because caller can reuse one variable over long loop, instead of each iteration creating new instance.

I would not generally use it, but eg. in game programing it has its place. For example look at JavaMonkey's Vector3f lot of operations allows to pass instance that should be modified and returned as result.

Answer 4

I don't think those are two equivalent pieces of code. In first case, you have to create the otherObject. You can modify existing instance in the second. Both have it's uses. Code smell would be preferring one over another.

Answer 5

It really does depend on the language.

In Java the second form may be an anti-pattern, but some languages treat parameter passing differently. In Ada and VHDL for example, instead of pass by value or by reference, parameters can have modes in, out, or in out.

It is an error to modify an in parameter or read an out parameter, but any changes to an in out parameter are passed back to the caller.

So the two forms in Ada (these are also legal VHDL) are

function MyObject2OtherObject(mo : in MyObject) return OtherObject is
begin
    ... Do the conversion
    return otherObject;
end MyObject2OtherObject;

and

procedure MyObject2OtherObject(mo : in MyObject; oo : out OtherObject) is
begin
    ... Do the conversion
    oo := ... the conversion result;
end MyObject2OtherObject;

Both have their uses; a procedure can return multiple values in multiple Out parameters while a function can only return a single result. Because the purpose of the second parameter is clearly stated in the procedure declaration there can be no objection to this form. I tend to prefer the function for readability. but there will be cases where the procedure is better, e.g. where the caller has already created the object.

Answer 6

The advantage of the second pattern is that it forces the caller to take ownership and responsibility for the created object. There is no question whether or not the method created the object or got it from a reusable pool. The caller knows that they are responsible for the lifetime and disposal of the new object.

The disadvantages of this method are:

1. Can not be used as a factory method, the caller must know the exact subtype of OtherObject it needs and construct it in advance.
2. Can not be used with objects that require parameters in their constructor if those parameters come from MyObject. OtherObject must be constructable without knowing about MyObject.

The answer is based on my experience in c#, I hope the logic translates to Java.

Answer 7

It does indeed seem smelly, and without seeing more context it's impossible to say for sure. There could be two reasons for doing this, although there are alternatives for both.

First, it's a concise way of implementing partial conversion, or leaving the result with default values if conversion fails. That is, you might have this:

public void ConvertFoo(Foo from, Foo to) {
    if (can't convert) {
        return;
    }
    ...
}

Foo a;
Foo b = DefaultFoo();
ConvertFoo(a, b);
// If conversion fails, b is unchanged

Of course, usually this is handled using exceptions. However, even if exceptions need to be avoided for whatever reason, there's a better way to do this - the TryParse pattern being one option.

Another reason is that it could be for purely consistency reasons, for example it's part of a public API where this method is used for all conversion functions for whatever reason (such as other conversion functions having multiple outputs).

Java's not great at dealing with multiple outputs - it can't have output-only parameters like some languages, or have multiple return values like others - but even still, you could use return objects.

The consistency reason is rather lame but sadly it may be the most common.

• Perhaps the style cops at your workplace (or your codebase) come from a non-Java background and have been reluctant to change.
• Your code may have been a port from a language where this style is more idiomatic.
• Your organisation may need to maintain API consistency across different languages, and this was the lowest-common-denominator style (it's daft but it happens even for Google).
• Or perhaps the style made more sense in the distant past and morphed into its current form (for example, it could have been the TryParse pattern but some well-intentioned predecessor removed the return value after discovering that nobody checked it at all).

Answer 8

Given the loose semantics -- myObjectToOtherObject could equally mean that you move some data from the first object to the second or that you convert it entirely anew, the second method seems more adequate.

However, if the method name were Convert (which it should be if we look at the "... do the conversion" part), I'd say the second method wouldn't make any sense. You don't convert a value into another value that already exists, IMO.