2

I'd like to get some advice on what would be a good approach at hiding the creation of an object graph. In the code below I would like SomeClass to not be directly responsible for the creation of the entire Thing object graph (all the objects in the CreateThing method. I don't have any control over any of the objects in the Thing graph (for a little context, they are all WCF objects)

Additionally, I feel like there is a code smell here where IConfigService is passed into SomeClass just to initialize a property in the Thing object graph. I'd like to have the IConfigService dependency removed from this class as it does not seem to be a direct concern of this class.

public class SomeClass : ISomeClass
{
    private readonly IFirstDependency _firstDependency;
    private readonly ISecondDependency _secondDependency;
    private readonly IConfigService _configService;

    public SomeClass(IFirstDependency, firstDependency, ISecondDependency secondDependency, IConfigService configService)
    {
        _firstDependency = firstDependency;
        _secondDependency = secondDependency;
        _configService = configService;
    }

    public List<Foo> GetFoo(BarDto bar)
    {
        Thing thing = CreateThing(bar);
        /// other code omitted
    }

    private Thing CreateThing(BarDto bar)
    {
        return new Thing()
        {
            Child = new Child()
            {
                SimpleProp = "simple",
                GrandChild = new GrandChild()
                {
                    Items = new List<Item>()
                    {
                        Item = new Item()
                        {
                            Prop1 = _configService.GetSettings...
                            Prop2 = $"{bar.This} {bar.That}",
                            Prop3 = $"{bar.MoreBar} {bar.EvenMoreBar}"
                        }
                    }
                }
            }
        };
    }
}

Should I hide the creation of the object graph behind an interface and have the implementation take the IConfigService dependency? This doesn't seem like a proper factory per se. Thoughts?

2

If you don't want SomeClass to be responsible for creating Thing instances, then supply it with something that is responsible:

public class SomeClass : ISomeClass
{
    private readonly IFirstDependency _firstDependency;
    private readonly ISecondDependency _secondDependency;
    private readonly Func<Thing, BarDto> _thingCreator;

    public SomeClass(IFirstDependency firstDependency, 
                     ISecondDependency secondDependency, 
                     Func<Thing, BarDto> thingCreator)
    {
        _firstDependency = firstDependency;
        _secondDependency = secondDependency;
        _thingCreator = thingCreator;
    }

    public List<Foo> GetFoo(BarDto bar)
    {
        var thing = _thingCreator(bar);
        /// other code omitted
    }
}

And if you are using a DI container that can't support injecting functions, then either stop using that DI container and use pure DI, or wrap it in an interface/class to help the container out:

public ThingCreater : IThingCreator
{
    public Thing CreateThing(BarDto bar)
    {
        ...
    }
}

public class SomeClass : ISomeClass
{
    private readonly IFirstDependency _firstDependency;
    private readonly ISecondDependency _secondDependency;
    private readonly IThingCreator _thingCreator;

    public SomeClass(IFirstDependency firstDependency, 
                     ISecondDependency secondDependency, 
                     IThingCreator thingCreator)
    {
        _firstDependency = firstDependency;
        _secondDependency = secondDependency;
        _thingCreator = thingCreator;
    }

    public List<Foo> GetFoo(BarDto bar)
    {
        var thing = _thingCreator.CreateThing(bar);
        /// other code omitted
    }
}
  • Just updated the example. Sorry for not being thorough enough. Added code to show there are more dependencies in constructor. Using the default .NET Core container for DI. – AvacadoRancher Mar 13 at 12:15
  • 1
    @AvacadoRancher, I've updated my answer to try and address your comments. But I'm still a little unsure what you are wanting to do, so I'm not sure I'm really answering your question. – David Arno Mar 13 at 14:11
  • Thanks David! This along with the answer from candied_orange confirmed my thinking. Thanks to the both of you for helping out, greatly appreciated. – AvacadoRancher Mar 13 at 17:11
0

First of all, you are doing something wrong in your CreateThing. As I see, this method is responsible to create new Child class. Then you don't need to create entire Thing class. Give this responsible to caller class.

Even you need to create and initialize your Thing class, you can write static helper class or add constructor to Thing class that what it need to initialize.

public class Thing
{
    private Child _child;
    public Thing(Child child)
    {
        _child = child;
    }
}

Besides, if getting List<Foo> is need to do many business, you can separate concerns(business) to different classes and get required object from there. Then, initialize your Thing object.

public List<Foo> GetFoo(BarDto bar)
{
    Child child = _service1.GetChild(bar);
    Child2 child2 = _service2.GetChild(bar);
    Thing thing = new Thing(child, child2);
    //..        
}

Additionally, I feel like there is a code smell here where IConfigService is passed into SomeClass just to initialize a property in the Thing object graph. I'd like to have the IConfigService dependency removed from this class as it does not seem to be a direct concern of this class

You need IConfigService to do your business. Injecting this interface is really better way instead of initializing right there. So, you are doing good. I think you are dissatisfied with injecting IConfigService in the constructor because you are using this just in this method. If so and if you use IoC container, then you can create new property to inject IConfigService.

private IConfigService ConfigService
{
   get
   {
       return Container.Context.GetInstance<IConfigService>()
   }
}

By doing this way, only initialize when you call ConfigService property. But it is not a big deal and no need to implement all injection by this way. Because this way depends on container.

Btw, if your SomeClass is your business class(not your Service Layer class), then do not use DTOs on your Busines Logic Layer(BLL). Use your own models instead. It is more important decoupling than injecting service for me.

0

With names like SomeClass and Thing we have no semantic clues of what goes where.

That leaves analyzing the dependencies. I don't see that CreateThing needs anything but bar and configService. configService is all CreateThing gets from SomeClass. configService (or Prop1 for that matter) could be passed directly to it if you want to separate SomeClass and CreateThing. It could be passed to the method or even a new type of object that holds it if you need to set that at a different time.

Whether any of that makes sense to do I've no clue with names like these. But yes the fact that refactorings like "extract method" exist proves you can do stuff like this if you really want.

Once you move CreateThing to a new type, say ThingFactory you can make that type part of SomeClass and call on it's CreateThing method at any time. Do this through a IThingFactory and you've laid the ground work for an Abstract Factory Pattern. Which simply means you can pass SomeClass different kinds of ThingFactorys and get different kinds of Thing graphs without having to change SomeClass.

No idea if that's useful to you but it does what you're asking for without painting you into a corner.

  • yes you make perfect sense. Sorry for the vagueness. Your answer and the answer provided by David Arno confirm my thinking. Just wasn't sure if factory was best way to go since there is no subclassing. Thanks to both of you, very helpful answers. – AvacadoRancher Mar 13 at 17:10
0

Assuming I understood your requirements i.e. you want to delegate the control of the creation of a object elsewhere.

There are several options:

  1. Use the Builder pattern

This is probably the cleanest way of handling this (based on your code above). You can implement any methods you need to control how the object gets built, should you need it, or can just have the base "Build" method that builds the default object structure.

The builder pattern is most appropriate when you want to provide an abstraction over the creation of a complex object.

Example:

public interface IThingBuilder
{
    Thing Build(BarDto dto);
}

public class ThingBuilder
{
    public Thing Build(BarDto dto)
    {
        //... insert your complex object construction process here
    }
}

public class SomeClass
{
    private IThingBuilder _thingBuilder;

    public SomeClass(IThingBuilder thingBuilder)
    { 
        _thingBuilder = thingBuilder;
    }

    public List<Foo> GetFoo(BarDto bar)
    {
        Thing thing = _thingBuilder.Build(bar);
        /// other code omitted
    }

}
  1. Use the Factory pattern

Assuming you don't want to build and overly complex object and you just want to provide multiple implementations of a base class or interface then the factory pattern is your best bet.

public interface IThingFactory
{
    public Thing GetThing(string type, BarDto dto);
}

public class ThingFactory : IThingFactory
{
    public Thing GetThing(string type, BarDto dto)
    {
        switch(type)
        {
            case "SomeThing": return new SomeThing(dto);
            case "SomeOtherThing": return new SomeOtherThing(dto);
            default: return new Thing(dto);
        }
    }
}

public class SomeClass
{
    private IThingFactory _thingFactory;

    public SomeClass(IThingFactory thingFactory)
    {
        _thingFactory = thingFactory;
    }

    public List<Foo> GetFoo(BarDto bar)
    {
        var thing = _thingFactory.GetThing("SomeThing", bar);
        /// other code omitted
    }
}
  1. Use the Strategy pattern

Do you need different concrete objects based on some condition? If so, combining the Factory pattern with a switch-case or if-else statements can provide you with a flexible way of creating instances of various concrete types that satisfy a known interface / base class.

public interface IThingFactory
{
    public Thing GetThing(string type, BarDto dto);
}

public class ThingFactory : IThingFactory
{
    public Thing GetThing(string type, BarDto dto)
    {
        switch(type)
        {
            case "SomeThing": return new SomeThing(dto);
            case "SomeOtherThing": return new SomeOtherThing(dto);
            default: return new Thing(dto);
        }
    }
}

public class SomeClass
{
    private IThingFactory _thingFactory;

    public SomeClass(IThingFactory thingFactory)
    {
        _thingFactory = thingFactory;
    }

    public List<Foo> GetFoo(BarDto bar)
    {
        Thing thing;

        if(string.IsNullOrEmpty(bar.PropertyA))
        {
            thing = _thingFactory.GetThing("SomeOtherThing", bar);     
        }
        else
        {
            thing = _thingFactory.GetThing("SomeThing", bar);
        }

        /// other code omitted
    }
}

Which one you'll end up using will depend on your specific requirements.

I hope this helps.

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.