I came across the following 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);
}
I'm not an OO guru, so take these with a grain of salt.
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.
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.