Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question

closed as primarily opinion-based by mattnz, gnat, Bart van Ingen Schenau, MichaelT, Ampt Jun 27 at 16:02

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise.If this question can be reworded to fit the rules in the help center, please edit the question.

2  
The right answer for this is going to be language-specific, since those where pass-by-value parameters effectively turn them into local variables. –  Blrfl Jun 23 at 14:36
1  
The second pattern is sometimes used as an efficiency measure to reuse objects. Assuming that oo is an object that is passed to the method, not a pointer that is set to a new object. Is that the case here? If this is Java it probably is, if its C++ it might not be –  Richard Tingle Jun 23 at 15:24
2  
This looks like premature optimization, aye. I recommend that you use the first form in general, but if you run into a performance bottleneck, you can use the second form with a comment to justify it. A comment would prevent yourself and others from looking at that code and having this same question pop up all over again. –  Cory Jun 23 at 16:01
1  
The second pattern is useful when the function returns a value as well, for ex, success/fail. But there's nothing really 'wrong' with it. It really depends on where you want to create the object. –  GrandmasterB Jun 23 at 16:49
10  
I would not name functions implementing these two different patterns the same way. –  emodendroket Jun 23 at 17:23

8 Answers 8

up vote 35 down vote accepted

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

The difference between an antipattern an a simple 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

share|improve this answer
4  
I don't see why the definition you linked to precludes this being considered an anti-pattern. –  Samuel Edwin Ward Jun 23 at 20:23
3  
I suppose it's because that the rationale that "we're saving CPU/mem by not creating new object, instead we should recycle one we have and pass it as an arg" is so obviously wrong for about everybody with serious OOP background that this could not constitute a pattern ever - so there's no way we could consider it an anti-pattern by the definition... –  vaxquis Jun 23 at 22:36

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.

share|improve this answer
9  
well, I can agree if this is the bottleneck, but wherever I worked, the bottlenecks are usually algorithms with low efficiency, not cloning or creating objects. I know less of game development. –  CsBalazsHungary Jun 23 at 14:28
2  
@CsBalazsHungary I believe the issues when object creation is a concern tend to be related to memory allocations and garbage collection, which would probably be the next level of bottlenecks following algorithmic complexity (especially in a limited-memory environment, e.g. smartphones and the like). –  JAB Jun 23 at 15:14
    
JMonkey is also where I have seen this a lot since it is often performance critical. I've never heard it called "JavaMonkey" though –  Richard Tingle Jun 23 at 15:27
3  
@JAB: The JVM's GC is tuned specifically for the case of ephemeral objects. Large quantities of very short lived objects are trivial to collect, and in many cases your entire ephemeral generation could be collected with a single pointer move. –  Phoshi Jun 23 at 15:51
1  
actually, if one has to create an object, it won't be performance-effective; if a code block has to be heavily optimized for speed, you should use primitives and native arrays instead; because of that, I consider that the statement in this answer doesn't hold. –  vaxquis Jun 23 at 22:39

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.

share|improve this answer
    
I know they are not equivalent. You are right, we are persisting these things, so it would make difference. So you are saying none of them are antipattern, it is just question of the use case? –  CsBalazsHungary Jun 23 at 12:01
    
@CsBalazsHungary I would say yes. Judging from the small piece of code you provided. –  Euphoric Jun 23 at 12:02
    
I am guessing it becomes more serious issue if you have an incoming primitive parameter, which of course won't be updated, so like @claasz wrote: it should be avoided unless it is neccessary. –  CsBalazsHungary Jun 23 at 12:20
1  
@CsBalazsHungary In case of primitive argument, the function wouldn't even work. So you have worse problem than changing arguments. –  Euphoric Jun 23 at 12:22
    
I would say a junior would fall into the trap of trying to make a primitive type an output parameter. So I would say the the first converter should be preferred if possible. –  CsBalazsHungary Jun 23 at 12:24

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.

share|improve this answer
1  
I would say it violates the first one, lot of arguments mean messy code, but I think two arguments are totally ok. I think that rule on clean code means if you need 5 or 6 incoming data, you do want to do too much in a single method, so you should refactor to increase the code readability and OOP scope. –  CsBalazsHungary Jun 23 at 12:10
2  
anyway I suspect this is not a strict law, but a strong suggestion. I know it won't work with primitive types, if it is a wide-spread pattern in a project a junior would get ideas to pass an int and expect it to be changed like Objects. –  CsBalazsHungary Jun 23 at 12:22
19  
Regardless of how correct Clean Code may be I don't think this is a valuable answer without explaining why these are to be avoided. They're not commandments coming down on a stone tablet, understanding is important. This answer could be improved by providing a summary of the reasoning in the book and a chapter reference –  Daenyth Jun 23 at 14:54
2  
Well, hell, if some guy wrote it in a book it must be good advice for any conceivable situation. No need for thought. –  emodendroket Jun 23 at 16:58
1  
@emodendroket But dude, it's the guy! –  Pierre Arlaud Jun 24 at 8:35

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.

share|improve this answer

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.

share|improve this answer

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).
share|improve this answer

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.

share|improve this answer
    
I think with the responsibility you mention, it also creates a "harder to see" lifecycle of objects. In a complex method system I would prefer a direct modification of an object rather than start to look through all the method we are passing. –  CsBalazsHungary Jun 23 at 12:47
    
This was what i was thinking. A primitive kind of Dependency Injection –  Oxinabox Jun 24 at 23:36

protected by gnat Jun 24 at 12:30

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.