0

What is preferred and why?

void PrepMyInstantiatedObject(SomeComplexDTO someComplexDto){
      someComplexDto.Name = SomeOtherClassHelper.DerivedName(someComplexDto.Name);
      someComplexDto.Ids = SomeOtherClassHelper.Resolve(someComplexDto.RelatedIds); 
      someComplexDto.SingleId = SomeOtherClassHelper.FindId(someComplexDto.Name);
}

or

SomeComplexDTO PrepMyInstantiatedObject(SomeComplexDTO someComplexDto){
      someComplexDto.Name = SomeOtherClassHelper.DerivedName(someComplexDto.Name); 
      someComplexDto.Ids = SomeOtherClassHelper.Resolve(someComplexDto.RelatedIds); 
      someComplexDto.SingleId = SomeOtherClassHelper.FindId(someComplexDto.Name);

      return someComplexDto;
}

Although returning the object is not necessary (2nd method) - someone was telling me it was easier to understand how to use the method PrepMyInstantiatedObject. Meanwhile I thought the name was sufficient in conveying its purpose.

public class SomeComplexDTO
{
    public string Name {get; set;}
    public IEnumerable<int> Ids {get; set;}
    public IEnumerable<int> RelatedIds {get; set;}
    public int SingleId {get; set;}
}

One could argue that this DTO should have the logic for prepping itself however my team does not want to bloat up this class with logic.

5

Neither. But the reasons are complex

Firstly, if you are coming from an OOP perspective, the Method should be on the object

SomeComplexDTO.Prep();

Secondly functions with side effects are also disapproved of. so from this angle you should have

SomeComplexDTO  PrepMyInstantiatedObject();

or

void PrepMyInstantiatedObject(out SomeComplexDTO someComplexDto)

Thirdly we have the 'immutables are good' philosophy, which would suggest

SomeComplexDTO PrepMyInstantiatedObject(SomeComplexDTO someComplexDto)
{
    return new SomeComplexDTO(
        SomeOtherClassHelper.DerivedName(someComplexDto.Name),
        SomeOtherClassHelper.Resolve(someComplexDto.RelatedIds),
        SomeOtherClassHelper.FindId(someComplexDto.Name)
        );
}

I think my main 'code smell' with both of your methods though is that they imply that the object you are passing in is 'half constructed' and in an invalid state.

Now I am not one to insist that objects should never be in an invalid state, but here you seem to be designing for that to be normal.

Refactor the code so that you have a single setup function and you wont need to pass an object into your setup methods, which removes most of the objections.

SomeComplexDTO GetObject(string name, string[] relatedIds, string id)
{
    return new SomeComplexDTO(
        name,
        relatedIds,
        id
        );
}
  • 1
    Given a particularly systemically buggy class I will sometimes do an initial refactor by parameterizing private/protected methods with an explicit return. When state is confusing as if in a blender on puree then this technique quickly brings understanding and predictability allowing for the actual fix to proceed. – radarbob May 25 '18 at 15:42
  • Good answer, but you may add that the "immutable" solution is intended for a design where the object has no public methods which change its state, and is sealed. – Doc Brown May 27 '18 at 6:45
2

If you are in doubt, always use the simplest solution. The simplest here is the first example, which does not return a superfluous value. For the second option users looking at the call site would wonder if the returned object is the same as the input object or what the difference is.

Just keep it simple.

  • Upvote for keeping it simple. I generally try to adhere to OOP principles but sometimes the author's original method is best, especially if you're working with generics. I've got a static method that takes any reference type object and sets any string property that's an empty string to null. I do this with lots of different classes. Sure, I could use class inheritance and scaffold it out in correct OOP style, but why? – Neil Laslett Nov 21 '18 at 16:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.