0

Changing parameter values is considered an anti-pattern, but I find it useful sometimes with optional parameters in C#:

public void Foo(int p1, MyClass fooObj = null)
{
    if (fooObj == null)
    {  
        fooObj = LoadFooObj(....
    }

    . . .
}

Is something here potentially harmful I may be missing?

Thanks.

Artur Udod
  • 4,465
  • 1
  • 29
  • 58
  • That's perfectly common and OK. The idea about not changing parameter values is probably aimed at the 100+ line methods that people used to write. If you only ever have short methods, it's not going to be an issue. – Matthew Watson Jul 28 '15 at 12:06

3 Answers3

4

If you think it smells, how about a simple overload instead.

public void Foo(int p1, MyClass fooObj)
{
    . . .
}

public void Foo(int p1)
{
    var fooObj = LoadFooObj(....);
    Foo(p1, fooObj);
}

This way its clear what each method does and your not altering the arguments inside the call.

Ash Burlaczenko
  • 24,778
  • 15
  • 68
  • 99
  • yep, your code looks nice, thanks. The point is I am not sure whether original code smells) – Artur Udod Jul 28 '15 at 12:07
  • The only problem with it is it is hard to sift out where it I being called without the option. If you seperate them u can easily find all references in the ide – Brandon Seydel Jul 28 '15 at 12:16
  • It's also tricky if you want to have multiple optional parameters - the number of overloads explodes nastily. – Jon Skeet Jul 28 '15 at 12:28
  • That's one of the reasons why I try to use Named Parameters Calls for Methods with over 1-2 Parameters and/or where the Parameter is not obvious from the name of the Method. Of course, that also helps avoid unintentional Parameters when one/more Parameters in the Signature are replaced with (and/or in the case of Params Parameters, removed leaving) Type-Compatible Parameters after Calls have been coded. In fact, I think there should be a "Parameter Explicit" Compiler option (a la "Option Explicit" in VB) that requires Named Parameters. See: https://stackoverflow.com/a/31145312/401246 – Tom May 08 '18 at 20:29
3

That's absolutely fine. In fact, it's a good way of making a parameter optional without having to bake in the value as a constant.

You can use the null-coalescing operator to make it slightly more readable though:

fooObj = fooObj ?? LoadFooObj();

You could even consider using the same approach for value types:

public void Log(string message, DateTime? timestamp = null)
{
    DateTime actualTimestamp = timestamp ?? DateTime.UtcNow;
    ...
}

One downside of this is that it prevents null from being used as a "normal" meaningful value though - consider whether or not you will ever need that in a particular context.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
2

I'm going to argue the complete opposite of Jon Skeet and say it's not fine for two reasons:

  1. As much as possible, all variables (including parameters) should be treated as being immutable. Only change their value if you really have to. This leads to clearer, easier to understand code.
  2. Avoid optional parameters. A method with an optional parameter that then immediately tests that parameter is a clear code smell: you have two routes through the code, so make it two methods.

You could just overload Foo, but think about what the methods do: they likely should be given different names to describe the fact that they do different things. Do not rely on comments to explain this; make it clear with the code itself.

David Arno
  • 42,717
  • 16
  • 86
  • 131
  • I don't see why (2) applies particularly to optional parameters. Surely the same logic would apply to any parameter regardless of whether it has a default value? – Matthew Watson Jul 28 '15 at 12:50
  • Does it also apply to any method with a boolean parameter then? I can just have two methods with slightly different names. – Artur Udod Jul 28 '15 at 13:33
  • @ArturUdod, absolutely this rule should be applied to boolean parameters. – David Arno Jul 28 '15 at 14:22
  • @MatthewWatson, you are right. Any parameter that is used as a test to choose between two different paths through the code is a "two methods likely needed here" smell. – David Arno Jul 28 '15 at 14:23