Returning a new Object vs modifying one passed in as a parameter

I came across the following开发者_JAVA百科 piece of code during a code review.

My intuition is telling me that this isn't following proper OOP.

I'm thinking that instead the LoadObject method should return a new SomeObject object, instead of modifying the one passed into it. Though I can't really find a proper explanation of why this is better.

Is my solution better? and if so why? specifically what OOP principles or standards are broken in the given code example (if any) ?

   public void someMethod()
    {
        ...
        var someObject = new SomeObject();
        LoadSomeObject(reader,someObject);
    }

    private void LoadSomeObject(SqlDataReader reader, SomeObject someObject)
    {
       someObject.Id = reader.GetGuid(0);
    }

There's nothing wrong with the way the code is written since you are only modifying a property on someObject.

However, it would be just as correct to create someObject within LoadSomeObject and return it.

At this point, both choices are just as correct.

I'm not an OO guru, so take these with a grain of salt.

1. objects should manage themself/define their behaviour themselves as far as possible. The rationale behind this is obvious if you ever head of loose coupling. I may very well be the better design decision to move the details of LoadSomeObject to the implementation of SomeObject, but that's hard to discuss for such a general example.

2. Mutable state is perfectly fine in any imperative code (including OO code), it's a core "feature" of these paradigms. OTOH, immutable state has undeinable advantages (I guess we have a few questions on that topic here, otherwise ask any FP advocate), and having some immutable objects is not especially non-OO.

Edit: You may as well pass reader to SomeObject's constructor.

There isn't universal OO concept, that is conflicting with code like that. But later you'll find that is hard to collect and understand way to manipulate of SomeObject instances if you will not follow some design principles. Maybe easiest way for starter is to separate two main kind of procedures:

• Functional - that is designed to create new instances and not to mutate other objects.
• Methodic - that is designed to change state of it's host instance.

So good idea here, if you want to separate SomeObject manipulate logic is to create SomeObjectBuilder type with

 public void loadFromReader( SqlDataReader reader )

method and

public SomeObject getValue()

property

Either way is perfectly acceptable but considerations about what you wish to do with the returned object come in to play when deciding which is right for your particular situation. Immutability i.e creating a new instance in every operation is how strings in .NET are handled.

A copy method would obviously need to return a copy. Where as a method that works on a singleton object for example that is held by a global reference would not suit returning a new object as the changes may be lost.

If LoadSomeObject is going to return a new SomeObject, you might want to change the method name to avoid confusion. Maybe to NewAndLoadSomeObject?

I do agree with your intuition, that it feels a bit off for most cases. I would agree that returning something is better, as just makes it more clear that something is being modified. Seeing a returned value being used makes it immediately clear what is going on.

I think this feels a little better still, though (in most cases anyway):

public void someMethod()
{
    ...
    var someObject = new SomeObject();
    someObject.Load(reader);
}

And then obviously in the SomeObject class, you would have

public void Load(SqlDataReader reader)
{
   this.Id = reader.GetGuid(0);
}

The idea is to favour instance methods over static ones. Why bother creating an object and passing its data around when you can just have the object operate on its own data?