13

So something like this 

public void MyMethod(object parameter)
//....
    BuildSomething(parameter);
    BuildLayers(parameter);
    BuildOtherStuff(parameter);
}

public void BuildSomething(object parameter)
{
//...
    parameter.SomeProperty = "sadsd";
//...
}

If this is an anti pattern, what is it called? The problem (possibly) is that you are implicitly changing parameter and using the changed value.
I just want to know what is this anti-pattern know as

Thanks

roundcrisis
  • 17,276
  • 14
  • 60
  • 92
  • 3
    I believe this is fine as long as a method name states what it does – sll Dec 28 '12 at 12:40
  • @sll that's where this one falls down. It either doesn't say what it does, or the method name is BuildSomethingwithLayersBuiltFromOtherStuff. – Tony Hopkinson Dec 28 '12 at 13:13

3 Answers3

21

It is a side effect.

These are normally not good and considered to be a code smell as it makes it difficult to reason about and understand code.

However, this pattern is sometimes useful.

C# codified the ref and out keywords specifically to show that a method is expected to have side effects.

Oded
  • 489,969
  • 99
  • 883
  • 1,009
  • 4
    @knaki02 i dont think so... the definition is very clear: `a function or expression is said to have a side effect if, in addition to returning a value, it also modifies some state or has an observable interaction with calling functions or the outside world`. – Leonardo Dec 28 '12 at 13:10
  • As far as I can see the fact that *parameter* has changed is an observable interaction with the outside world – roundcrisis Dec 28 '12 at 13:21
  • 1
    What about 'in addition to returning a value' ? Void method does not. – knaki02 Dec 28 '12 at 13:34
  • 5
    It's a side effect, because it changes the argument and it's not clear that this is what the method does. Never mind what some academics say, void DoSomethingTo(arg) is not DoSomethingWith(arg). – Tony Hopkinson Dec 28 '12 at 14:03
  • Returning a void result doesn't imply a side-effect, in the same way that non-void return types don't imply purity. It just-so-happens that void only contains one value (NULL), hence it contains no runtime information (unlike, say, bools which contain 1 bit). One place where it's useful to pass around voids is implementing an interface parametrised on a type (ie. using existential types). For example, our interface may be parameterised on a "Config" type and requires us to implement a function "getConfig : void -> Config"; if our implementation is unconfigurable, we can use void as Config. – Warbo Jul 10 '14 at 16:45
  • theoretically speaking, void IS a return value. it's type contains only one value, and it's kind of similar to unit in F#. in C# you can't assign it to anything though. in a purely functional sense, a method returning void makes no sense or is just really uninteresting, it's equivalent to no code at all. it can only be of use if it has some side effects. like this answer makes clear though, side effects can be tricky, and they are one of the reasons imperative code can be really hard to follow without knowing the whole or large parts of the system. – sara Apr 05 '16 at 08:10
0

I have a different point of view.

Despite the little problems that change parameters value can introduce in debugging process or code readability, it don't make sense to me call this practice as "anti-pattern".

Based on modern OO-languages design like Java or C#, I support the idea that, if it is ugly, wrong or not recommendable to change parameters values, they'd have done defined type parameters as copy of instances, rather than references.

And disagreeing with what Oded said, I think ref or out keyword should be used only in contexts where you really want to change the entire instance value, replacing it completely. To use one of this keywords just to tell "hey guy, the parameters values can change during the execution stack" sounds a little bit careless to me. What if one of your clients see the function signature and really believes that he can replace the entire thing? (in a situation that is not expected behavior).

João Paulo Navarro
  • 544
  • 1
  • 7
  • 16
  • I'd rather see ref or out based on the op's example as MyObject DoSomethingWith(MyObject oldObj); is a lot clearer. As essentially a maintenance coder, I loathe side effects. And if it is a out or a ref they can replace the entire thing, they know that, the code should cope with that. Not believing the method will change the argument is a much bigger problem in my experience, it should only be changing the class the argument is being passed to. – Tony Hopkinson Dec 28 '12 at 13:50
  • This smells of dogma and cargo-cult to me http://en.wikipedia.org/wiki/Argument_from_authority – Warbo Jul 10 '14 at 16:47
0

Assuming the type of parameter isn't really object, but rather is a class type which contains a writable property or field called SomeProperty, then when the method enters, the value of parameter will be the identity of some object (say, the 459,192nd object created since the program began). So far as I can tell, the value of that parameter (meaning the identity of the object to which it refers) will continue to be the same throughout the method.

Changing the value of the passed-in parameter (e.g. saying parameter = someOtherObject) may be a code smell unless the method is sufficiently small that it's obvious what's going on.

supercat
  • 77,689
  • 9
  • 166
  • 211