Are void return methods that change the state of their argument an anti-pattern?

Question

Are methods that return void but change the state of their arguments (ie. provide a hidden or implicit return value) generally a bad practice?

I find them difficult to mock, which suggests they are possibly a sign of a bad design.

What patterns are there for avoiding them?

A highly contrived example:

public interface IMapper
{
    void Map(SourceObject source, TargetObject target);
}

public class ClassUnderTest
{
    private IMapper _mapper;

    public ClassUnderTest(IMapper mapper)
    {
        _mapper = mapper;
    }

    public int SomeOperation()
    {
        var source = new SourceObject();
        var target = new TargetObject();

        _mapper.Map(source, target);

        return target.SomeMappedValue;
    }
}

How about void Initialize() method, that returns void and exists only for the purpose of changing object state? Or void Fill(object o) that do some action on the object o? Return type has no correlation with internal state and arguments. The semantic of method should clearly indicate whether state or arguments will be changed. If method does something more, that it is intended, then it is an anti pattern. It is more about clean coding. — Ryszard Dżegan, Mar 12 '13 at 13:55
That's exactly what I was driving at - the semantics aren't clear, or are at least muddled in these cases. — MalcomTucker, Mar 12 '13 at 13:57

score 2 · Answer 1 · answered Mar 12 '13 at 13:56

Your code whould be a lot easier to test if you do this:

public interface IMapper
{
    TargetObject Map(SourceObject source);
}

public class ClassUnderTest
{
    private IMapper _mapper;

    public ClassUnderTest(IMapper mapper)
    {
        _mapper = mapper;
    }

    public int SomeOperation(SourceObject source )
    {
        var target =  _mapper.Map(source, target);
        return target.SomeMappedValue;
    }
}

You can now test you Map opperation and SomeOperation seperatly. The problem is that you idd change the state of an object which makes it hard to provide a stub for testing. When returning the new object you are able to return a test stub of the target and test your caller method.

Yeah absolutely, the change above is utterly trivial. But sometimes you can't change an API so easily. My question is more whether people consider this kind of pattern a bad design... — MalcomTucker, Mar 12 '13 at 13:58
If you can't test it, it's bad design. It is all about writing testable code. And if it is testable, it is much easier to understand. — Peter, Mar 12 '13 at 13:59

score 2 · Accepted Answer · answered Mar 12 '13 at 15:48

Yes to some extend.

What you describe is a typical side effect. Side effects make programs hard to understand, because the information you need to understand isn't contained in the call stack. You need additional information, i.e. what methods got called before (and in what) order.

The solution is to program without side effects. This means you don't change variables, fields or anything. Instead you would return a new version of what you normally would change.

This is a basic principle of functional programming.

Of course this way of programming has it's own challenges. Just consider I/O.

Are void return methods that change the state of their argument an anti-pattern?

2 Answers2