Maximizing code readability in methods with side effects?

Question

I'm working with a method that causes side effects on a passed reference parameter. One of my primary concerns for the code is readability, and this is a situation that I feel is a potential cause for confusion.

Take for example:

class Program
{
    static void Main(string[] args)
    {
        var simplePOCO = new SimplePOCO();

        // Method 1: causes side effects, but potentially unclear
        Method1(simplePOCO);

        // Method 2: assignment makes intentions clearer, but unnecessary
        simplePOCO = Method2(simplePOCO);

        // Method 3: ref/out makes intentions clearer, but (very?) unnecessary
        Method3(ref simplePOCO);

        // Method 4: avoid the problem altogether
        simplePOCO.SimpleProperty = Method4();
    }

    public static void Method1(SimplePOCO simplePOCO)
    {
        simplePOCO.SimpleProperty = 1;
    }

    public static SimplePOCO Method2(SimplePOCO simplePOCO)
    {
        simplePOCO.SimpleProperty = 1;
        return simplePOCO;
    }

    public static SimplePOCO Method3(ref SimplePOCO simplePOCO)
    {
        simplePOCO.SimpleProperty = 1;
        return simplePOCO;
    }

    public static int Method4()
    {
        return 3;
    }
}

class SimplePOCO
{
    public int SimpleProperty { get; set; }
}

I'm leaning toward using method 2, but I realize it's just using a self-assignment. Method 4 also looks good but in this case will take a little refactoring to get there - is it worth it? I'm curious if anyone has any strong feelings one way or another about this. Obviously, proper naming of the method will go a long way. Are there or some school of thought that addresses this concern? Are there any other styles I haven't thought of?

Do not waste your time trying to optimize silly things like this. What exactly is wrong with the first option? The fact its even a static void is what I would worry about. — Security Hound, Nov 03 '11 at 18:27
static keyword was necessary for the example to compile. I wasn't optimizing, but rather looking for a rule of thumb for the future. — Chris Trombley, Nov 03 '11 at 18:33
It's hard to give general advice on a problem that is so abstracted. Consider adding more real-world detail. Having said that, method 3 is almost certainly the wrong way to go (see [(1)](http://msdn.microsoft.com/en-us/library/ms182131%28v=VS.100%29.aspx), [(2)](http://msdn.microsoft.com/en-us/library/ms182146.aspx)). — ladenedge, Nov 03 '11 at 18:35
Option 2 is much more confusing than option 1. By returning the mutated instance, you're suggesting that the original instance -- the parameter -- *isn't* changed, and that someone could, say, save off the reference before the method call to keep a "before" snapshot. Don't do this. If option 1 isn't clear enough, then you need a better method name, not a more arcane calling convention. — Joe White, Nov 03 '11 at 18:36

score 7 · Accepted Answer · answered Nov 03 '11 at 18:23

If this is the only side effect I would put the method were it belongs: inside SimplePOCO so that the method and the data it modifies are encapsulated together:

class SimplePOCO
{
    public int SimpleProperty { get; set; }
    public void Method5()
    {
       SimpleProperty = 3;
    }
}

Also the method name should indicate that a change is to be expected as a result of the call, i.e. UpdateSimplePropertyRandomly().

score 1 · Answer 2 · answered Nov 03 '11 at 18:31

1

I would go with:

public static void Method1(SimplePOCO simplePOCO)

If you return the object, I think it looks as if it's creating a new instance. To me, the void suggests the method is working on my reference that I pass in.

answered Nov 03 '11 at 18:31

Lee Gunn

8,417
4
38
33

score 0 · Answer 3 · answered Nov 03 '11 at 18:23

0

Why cant you create methods against SimplePOCO? Then you'd get

// npn-static
simplePOCO.Method1()
// static
SimplePOCO.Method1(simplePOCO)

answered Nov 03 '11 at 18:23

XeroxDucati

5,130
2
37
66

Maximizing code readability in methods with side effects?

3 Answers3