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 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();
// ...
Foo(something);
int result = Bar(something, 42);
Baz(something);

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)
something3.Apply(changes);

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?

share|improve this question
1  
This is a Side effect –  Dave Hillier Aug 19 '13 at 22:01
    
@DaveHillier Thanks, I was familiar with the term, but hadn't made the connection. –  michielvoo Aug 20 '13 at 8:26

8 Answers 8

up vote 3 down vote accepted

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.

share|improve this answer
    
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 Aug 19 '13 at 19:24

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 have method names like Foo and Bar, but methods which make clear that they change the object, I don't see a problem here. Think of

void AddMessageToLog(Logger logger, string msg)
{
    //...
}

or

void StripInvalidCharsFromName(Person p)
{
// ...
}

or

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

or

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.

share|improve this answer
    
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. –  michielvoo Aug 20 '13 at 8:27
1  
@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 Aug 20 '13 at 10:28

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.

share|improve this answer

Yes, see http://codebetter.com/matthewpodwysocki/2008/04/30/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.

share|improve this answer
    
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. –  Robert Harvey Aug 19 '13 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 Aug 19 '13 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. –  Robert Harvey Aug 19 '13 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 Aug 19 '13 at 22:34
    
I would say it's a mix of unexpected and non-obvious side effects. Thanks for the link. –  michielvoo Aug 20 '13 at 8:30

Done correctly, that pattern is called Dependency Injection. It's popular because it makes unit testing much easier compared to creating a Thing internally to another method, or maintaining a global reference to a Thing.

However, like any design principle, it's possible to abuse and implement incorrectly. Foo, Bar, and Baz might more properly be methods of a Thing class, for example. The difference is if Thing is the object being primarily acted upon, or if it's merely a dependency of the real work going on in those methods.

I don't know how it's done in C#, but in C++ we use const to designate when a passed in reference may not be changed, and the compiler enforces it.

share|improve this answer
1  
Just because one has a method with an object as a parameter, this is not automatically "DI" - If one hears hooves, better think of horses, not zebras just because zebras are popular these days ;-) –  Doc Brown Aug 19 '13 at 19:17
    
That's why I said "done correctly." It's impossible to tell from a foo-style example if DI was the actual intent or not. –  Karl Bielefeldt Aug 19 '13 at 19:19
2  
Done correctly or not, I don't see any DI pattern here. Just because DI is one of many, many situation where you need to pass objects into functions, you should not make the wrong assumption that whenever objects passed into functions, it is DI. Seems like a fallacy of false implication to me. –  Doc Brown Aug 19 '13 at 19:27
    
I don't think it qualifies as DI, sorry if my example is a bit generic to make that impression. –  michielvoo Aug 20 '13 at 8:29

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.

share|improve this answer

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.

share|improve this answer
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. –  Robert Harvey Aug 19 '13 at 21:00
1  
@RobertHarvey +1, good observation, I agree, will edit it. –  CMR Aug 23 '13 at 13:38

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();

excludeAdminUsersFilter.Apply(users);
filterByAnotherCriteria.Apply(users); 

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();
Filter(users);

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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