I am reading about common code smells in Martin Fowler's Refactoring book. In that context, I was wondering about a pattern I am seeing in a code base, and wether one could objectively consider it an anti-pattern.

The pattern is one where a object is passed as an argument to one or more methods, all of which change the object's state, but none of which return the object. So it is relying on the pass by reference nature of (in this case) C#/.NET.

var something = new Thing();
// ...
int result = Bar(something, 42);

I find that (especially when methods are not named appropriately) I need to look into such methods to understand if the object's state has changed. It makes code comprehension more complex, since I need to track multiple levels of the call-stack.

I'd like to propose to improve such code to return another (cloned) object with the new state, or anything that is needed to change the object at the call-site.

var something1 =  new Thing();
// ...

// Let's return a new instance of Thing
var something2 = Foo(something1);

// Let's use out param to 'return' other info about the operation
int result;
var something3 = Bar(something2, out result);

// If necessary, let's capture and make explicit complex changes
var changes = Baz(something3)

To me it seems the first pattern is chosen on the assumptions

  • that it is less work, or requires less lines of code
  • that it allows us to both change the object, and return some other piece of information
  • that it is more efficient since we have less instances of Thing.

I illustrate an alternative, but to propose it, one needs to have arguments against the original solution. What, if any, arguments can be made to make the case that the original solution is an anti-pattern?

And what, if anything, is wrong with my alternative solution?


7 Answers 7


Yes, the original solution is an anti-pattern for the reasons you describe: it makes it hard to reason about what is going on, the object is not responsible for its own state/implementation (breaking encapsulation). I would also add that all of those state changes are implicit contracts of the method, making that method fragile in the face of changing requirements.

That said, your solution has some of its own downsides, the most obvious of which is that cloning objects isn't great. It can be slow for large objects. It can lead to errors where other parts of the code hold on to the old references (which is likely the case in the codebase you describe). Making those objects explicitly immutable solves at least a few of these issues, but is a more drastic change.

Unless the objects are small and somewhat transient (which makes them good candidates for immutability), I would be inclined to simply move more of the state transition into the objects themselves. That lets you hide the implementation details of these transitions and set stronger requirements around who/what/where those state transitions occur.

  • 1
    For example, if I have a "File" object, I would not try to move any state changing method into that object - that would violate the SRP. That keeps valid even when you have your own classes instead of a library class like "File" - putting every state transition logic into the object's class does not really make sense.
    – Doc Brown
    Commented Aug 19, 2013 at 19:24
  • @Tetastyn I know this is old answer, but I'm having trouble visualizing your suggestion in the last paragraph in concrete terms. Could you elaborate or give an example?
    – AaronLS
    Commented May 24, 2019 at 16:47
  • 1
    @AaronLS - Instead of Bar(something) (and modifying the state of something), make Bar a member of something's type. something.Bar(42) is more likely to mutate something, while also allowing you to use OO tools (private state, interfaces, etc) to protect something's state
    – Telastyn
    Commented May 24, 2019 at 20:39

when methods are not named appropriately

Actually, that is the real code smell. If you have a mutable object, it provides methods to change its state. If you have a call to such a method embedded in a task of some more statements, it is fine to refactor that task to a method of its own - which leaves you in exact the situation described. But if you don't pick method names like Foo and Bar for the new method, but names which make clear that they change the object, I don't see a problem here. Think of

void AddMessageToLog(Logger logger, string msg)


void StripInvalidCharsFromName(Person p)
// ...


void AddValueToRepo(Repository repo,int val)
// ...


void TransferMoneyBetweenAccounts(Account source, Account destination, decimal amount)
// ...

or something like that - I don't see any reason here to return a cloned object for those methods, and there is also no reason to look into their implementation to understand that they will change the state of the object passed.

If you don't want side effects, make your objects immutable, it will enforce methods like the above ones to return a changed (cloned) object without changing the original one.

  • You are right, the rename method refactoring can improve the situation by making side-effects clear. It can become hard though, if the modifications are such that a concise method name is not possible. Commented Aug 20, 2013 at 8:27
  • 2
    @michielvoo: if consise method naming seems not to be possible, your method groups the wrong things together instead of building a functional abstraction for the task it does (and that is true with or without side effects).
    – Doc Brown
    Commented Aug 20, 2013 at 10:28
  • So you're basically implying that it is okay for private methods to have those side effects if their name conveys that, while it would probably won't be cool for public methods to modify their input parameters. Is that correct?
    – BornToCode
    Commented Jan 18, 2021 at 11:22
  • @BornToCode: no. Appropriate naming and visibility (public / private) are orthogonal to each other. What in my answer gives you the impression I could have meant something like that?
    – Doc Brown
    Commented Jan 18, 2021 at 11:50
  • I understood it from this sentence - If you have a call to such a method embedded in a task of some more statements, it is fine to refactor that task to a method of its own - which leaves you in exact the situation described. - so I understood that in a good design usually indeed a method shouldn't modify the input parameters but rather return a value. But when you refactor a big flow to small tasks (usually extracting single big method of a class to smaller private methods) you might find yourself in this situation (of methods modifying the input parameters).
    – BornToCode
    Commented Jan 18, 2021 at 12:03

Yes, see https://weblogs.asp.net/podwysocki/side-effecting-functions-are-code-smells for one of many examples of people pointing out that unexpected side effects are bad.

In general the fundamental principle is that software is built in layers, and each layer should present the cleanest possible abstraction to the next one. And a clean abstraction is one where you have to keep as little as possible in mind to use it. That's called modularity, and applies to everything from single functions to networked protocols.

  • I would characterize what the OP is describing as "expected side-effects." For example, a delegate you can pass to an engine of some sort that operates on each element in a list. That's basically what ForEach<T> does. Commented Aug 19, 2013 at 20:49
  • @RobertHarvey The complaints about methods not being named properly, and about having to read code to figure out the side effects, makes them definitely not expected side-effects.
    – btilly
    Commented Aug 19, 2013 at 21:41
  • I'll grant you that. But the corollary is that a properly named documented method with expected site effects might not be an anti-pattern after all. Commented Aug 19, 2013 at 22:07
  • @RobertHarvey I agree. The key is that significant side effects are very important to know about, and need to be documented carefully (preferably in the name of the method).
    – btilly
    Commented Aug 19, 2013 at 22:34
  • I would say it's a mix of unexpected and non-obvious side effects. Thanks for the link. Commented Aug 20, 2013 at 8:30

First of all, this doesn't depend upon the "pass by reference nature of" it depends upon objects being mutable reference types. In non-functional languages that's almost always going to be the case.

Secondly, whether this is a problem or not, depends upon both the object and how tightly the changes in the different procedures are tied together -- if you fail to make a change in Foo and that causes Bar to crash, then it's a problem. Not necessarily a code smell, but it's a problem with either Foo or Bar or Something (probably Bar as it should be checking it's input, but it could be Something being put into an invalid state which it should be preventing).

I wouldn't say that it rises to the level of an anti-pattern, but rather something to be aware.


I'd argue there's little difference between A.Do(Something) modifying something and something.Do() modifying something. In either case, it ought to be clear from the name of the invoked method that something will be modified. If it's not clear from the method name, regardless of whether something is a parameter, this, or part of the environment, it should not be modified.


I think it's fine to change state of the object in some scenarios. For example, I have a list of users and I want to apply different filters to the list before returning it to the client.

var users = Dependency.Resolve<IGetUsersQuery>().GetAll();

var excludeAdminUsersFilter = new ExcludeAdminUsersFilter();
var filterByAnotherCriteria = new AnotherCriteriaFilter();


And yes, you can make this pretty by moving filtering into another method, so you'll end up with something on the lines of:

var users = Dependency.Resolve<IGetUsersQuery>().GetAll();

Where Filter(users) would execute above filters.

I don't remember where exactly I came across this before, but I think it was referred to as filtering pipeline.


I am not sure if the proposed new solution (of copying objects) is a pattern. The problem, as you have pointed out, is the bad nomenclature of functions.

Let us say I write a complex mathematical operation as a function f(). I document that f() is a function that maps NXN to N, and the algorithm behind it. If the function is named inappropriately, and not documented, and has no accompanying test cases, and you will have to understand the code, in which case the code is useless.

About your solution, some observations:

  • Applications are designed from different aspects: when an object is used only to hold values, or is passed across component boundaries, it is wise to change the object's innards externally rather than fill it with details of how to change.
  • Cloning objects lead to bloating memory requirements, and in many cases lead to the existence of equivalent objects in incompatible states (X became Y after f(), but X is actually Y,) and possibly temporal inconsistency.

The problem you are trying to address is a valid one; however, even with enormous overengineering done, the problem is circumvented, not solved.

  • 2
    This would be a better answer if you related your observation to the OP's question. As is, it's more a comment than an answer. Commented Aug 19, 2013 at 21:00
  • 1
    @RobertHarvey +1, good observation, I agree, will edit it.
    – CMR
    Commented Aug 23, 2013 at 13:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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