1

Here's the situation: I am trying to determine the where part of cloning an object to avoid modifying the original.

I have two options:

  • Clone the object in the caller and pass the cloned object to a method ("callee"), thus preventing potential modification by the callee.
  • Clone the object in the callee because the callee modifies the object it is passed, this making the assumption that the caller never wants the argument object to be modified.

I found this 6-year-old answer with a variety of opinions. Unfortunately it doesn't seem there was a real consensus.

Passing copy of object to method -- who does the copying?

Here's my question in code-form:

  • Do I clone the object in the caller and pass the cloned object to the method?
public static void Main()
{
    var foo = new Foo();
    Bar.Baz(foo.DeepClone());
}

public static class Bar
{
    public static void Baz(Foo foo)
    {
        /* ... modifies members in foo ... */
    }
}

public class Foo { /* ... */ }
  • Do I clone the object in the callee?
public static void Main()
{
    var foo = new Foo();
    Bar.Baz(foo);
}

public static class Bar
{
    public static void Baz(Foo foo)
    {
        foo = foo.DeepClone();
        /* ... modifies members in foo ... */
    }
}

public class Foo { /* ... */ }

So, my questions are:

  • What are some good rules of thumb for where to clone objects across languages, but especially in C# and .NET-land?

  • Regardless of the answer(s), what are some good ways to document the behavior of methods that modify arguments, or methods that clone objects?

Community
  • 1
  • 1
aholmes
  • 458
  • 9
  • 20
  • This question belongs on http://codereview.stackexchange.com/. – Keith Payne Jun 17 '15 at 22:38
  • 2
    Code Review states "If your question is not about a particular piece of code and instead is a generally applicable question about…" ... "Best practices in general (that is, it's okay to ask "Does this code follow common best practices?", but not "What is the best practice regarding X?")" ... "then your question is off-topic for this site." – aholmes Jun 17 '15 at 22:45
  • 1
    This would not be a good fit for Code Review in its current form. The code is hypothetical; we require real code on CR. If you wish for a code review, please include all the real code you'd like to be reviewed. – Phrancis Jun 17 '15 at 22:58

2 Answers2

2

Is the purpose of the method to mutate the object? Then don't clone inside of the method. You want the side-effects to happen. Usually, the method name would clearly call out the fact that a mutation is expected (e.g. UpdateCustomer).

If it is not the explicit purpose of the method to mutate its inputs then the mutation is an implementation detail and the method must see to it that the mutation does not leak out. It can do that by cloning.

Methods should not use their inputs as mere scratch space. Some in the Win32 APIs do that which is horribly confusing.

usr
  • 168,620
  • 35
  • 240
  • 369
  • 1
    These are good hints, and stated clearly. The distinction between the caller expecting a mutation or not is the part I was missing. I'm curious about the Win32 APIs that do this. Got any resources? – aholmes Jun 17 '15 at 23:29
  • I couldn't find it when writing the answer. I read it on Raymond Chens blog. That API is decades old, it was a command line parsing API I think. It mutated the input string buffer and simply destroyed it in the process. – usr Jun 17 '15 at 23:32
  • Got it. It was the command line parameter of `CreateProcess`. – usr Jun 17 '15 at 23:34
  • Thanks! Pretty interesting to see issues like that. – aholmes Jun 18 '15 at 00:08
  • If you're interested in another thought: Generally you want to keep the contract between caller and callee (actually between any two modules) as clean as possible. Mutation guarantees are part of the contract. If you force developers to understand complicated mutation behavior that slows them down and causes potential for mistakes. Command query separation for instance aims at removing the need to understand mutation behavior from query methods. Generally the less mutation the better. And if you must mutate hide that fact from other modules. – usr Jun 18 '15 at 00:11
  • That makes sense. I just glanced at the wiki page for Command-query separation. If I understand you correctly, the idea is that, if I need to mutate a parameter, don't mutate the original *unless* the method's purpose is to explicitly mutate that parameter. Coincidentally, the real-world code that prompted this question is for querying a data store, and my "Bar.Baz()" method is a query method for which I don't want the internal mutations to leak. – aholmes Jun 18 '15 at 00:59
1

The best way to enforce (and document) constness is to define a read only interface and define your parameter as that interface. Anything that accepts the interface is constant and anything that accepts the full object might mutate the object.

If you are following this approach, the caller should be cloning if it does not want the side effects since we have given permission to the callee to modify the object by passing it a modifiable object.

Yaur
  • 7,333
  • 1
  • 25
  • 36
  • Is it reasonable in a language like C# (as opposed to, say, F#) to treat all objects as if they may be mutated at any time if they aren't explicitly read only? I see the value in implementing the read only interfaces, but it seems like a lot of overhead to provide that guarantee. – aholmes Jun 17 '15 at 23:32