5
\$\begingroup\$

I'm working with the unit of work pattern to glue together my application logic. To create such a unit of work object I use the builder pattern.

But my unit of work classes need access to the service collection of the dependency injection container but as the builder pattern requires you to know how to new the object this won't work. That's why I wrote the Build<T> method on MyUnitOfWork which uses reflection and the service container to get the parameters to construct the unit of work.

I know that I could remove MyUnitOfWorkConfiguration from the unit of work constructors but in the code I'm using this MyUnitOfWorkConfiguration class is quite big and the change would mean I need to duplicate the properties into the builder and the unit of work class.

public class MyUnitOfWorkConfiguration
{
    public bool RunSynchronously { get; set; }
}
public class SpecialUnitOfWorkConfiguration : MyUnitOfWorkConfiguration
{
    public bool IgnoreSpecialError { get; set; }
}

public abstract class MyUnitOfWork<TConfiguration> where TConfiguration : MyUnitOfWorkConfiguration
{
    protected readonly TConfiguration Configuration;

    protected MyUnitOfWork(IMyDependencyIncectedService myDependencyIncectedService, TConfiguration configuration)
    {
        // do something with the myDependencyIncectedService
        Configuration = configuration;
    }

    protected void ExecuteInner(Action action)
    {
        if (Configuration.RunSynchronously)
            action();
        else
            Task.Run(action);
    }
}
public class LoginUnitOfWork : MyUnitOfWork<MyUnitOfWorkConfiguration>
{
    public LoginUnitOfWork(IMyOtherDependencyIncectedService myOtherDependencyIncectedService, IMyDependencyIncectedService myDependencyIncectedService, MyUnitOfWorkConfiguration configuration)
        : base(myDependencyIncectedService, configuration)
    {
        // do something with the myOtherDependencyIncectedService
    }

    public void Login(string username, string password)
    {
        ExecuteInner(() =>
        {
            // do the login
        });
    }
}
public class SpecialUnitOfWork : MyUnitOfWork<SpecialUnitOfWorkConfiguration>
{
    public SpecialUnitOfWork(IMyDependencyIncectedService myDependencyIncectedService, SpecialUnitOfWorkConfiguration configuration) 
        : base(myDependencyIncectedService, configuration) { }

    public void Execute()
    {
        ExecuteInner(() =>
        {
            try { /* do special action */ }
            catch (SpecialException)
            {
                if (!Configuration.IgnoreSpecialError) throw;
            }
        });
    }
}

public abstract class MyUnitOfWorkBuilder<TConfiguration> where TConfiguration : MyUnitOfWorkConfiguration, new()
{
    protected readonly TConfiguration Configuration = new TConfiguration();

    public MyUnitOfWorkBuilder<TConfiguration> RunSynchronously()
    {
        Configuration.RunSynchronously = true;
        return this;
    }

    public T Build<T>() where T : MyUnitOfWork<TConfiguration>
    {
        var constructor = typeof(T).GetConstructors().First();
        var parameters = constructor.GetParameters();
        var parameterValues =
            parameters.Select(
                parameter =>
                    parameter.ParameterType.IsAssignableFrom(typeof(MyUnitOfWorkConfiguration))
                        ? Configuration
                        : ServiceLocator.Current.GetInstance(parameter.ParameterType));

        return (T)constructor.Invoke(parameterValues.ToArray());
    }
}
public class MyUnitOfWorkBuilder : MyUnitOfWorkBuilder<MyUnitOfWorkConfiguration> { }
public class SpecialUnitOfWorkBuilder : MyUnitOfWorkBuilder<SpecialUnitOfWorkConfiguration>
{
    public SpecialUnitOfWorkBuilder IgnoreSpecialError()
    {
        Configuration.IgnoreSpecialError = true;
        return this;
    }
}

var loginUnitOfWork = new MyUnitOfWorkBuilder()
    .RunSynchronously()
    .Build<LoginUnitOfWork>();
loginUnitOfWork.Login("myUserName", "myPassword");

var specialUnitOfWork = new SpecialUnitOfWorkBuilder()
   .IgnoreSpecialError()
   .RunSynchronously()
   .Build<SpecialUnitOfWork>();
specialUnitOfWork.Execute();

Examples for such a unit of work are:

  • ExitApplicationUnitOfWork (logs out users, cleans up, asks if the user is sure etc.)
  • LoginUserUnitOfWork (logs the user in)
  • LogoutUserUnitOfWork (logs the user out)
  • BackupUnitOfWork (backs up stuff)

At the moment there are about 8 of those unit of work types and after the implementation of the new feature set there will be around 15.

My questions are

  • Is what I do bad practice?
  • Do I follow the rules of the builder, unit of work and dependency injection pattern? Or is what I did here something strange I came up with?^^
  • Could I solve this better?

P.S.: If you have any other inputs to my code please just let me know.

\$\endgroup\$
  • 2
    \$\begingroup\$ There are a lot of mys. This does not look like real code. \$\endgroup\$ – t3chb0t Apr 27 '17 at 12:30
  • \$\begingroup\$ @t3chb0t the "my" is just a replacement of the products name \$\endgroup\$ – NtFreX Apr 27 '17 at 12:31
  • \$\begingroup\$ @t3chb0t and the SpecialUnitOfWork is an example \$\endgroup\$ – NtFreX Apr 27 '17 at 12:35
  • 1
    \$\begingroup\$ How many different configuration and 'unit of work' types do you have (or expect)? \$\endgroup\$ – JanDotNet Apr 27 '17 at 20:04
  • 1
    \$\begingroup\$ @JanDotNet now there are 8. But after the implementation of the new featureset there will be around 15. \$\endgroup\$ – NtFreX Apr 27 '17 at 20:05
1
\$\begingroup\$

As t3chb0t mentioned in a comment, the code is hard to review because it contains only the the structural code without any functional stuff.

Maybe that is the reason that it looks a little bit over-engineered to me. I'll try it anyway.. however probably the review does not fit to your real code ;)

In my eyes, the builder has no functionality but providing a nice fluent API for initializing the configuration object and creating the UnitOfWork object. The second part requires the access to a service locator (with has actually nothing to do with dependency injection).

So, first of all I would drop the builder and create the configuration object manually (alternatively you could use the builder only for building the the configuration object and pass it to the unit of work in a separate step):

var config = new ConfigurationObjectA();
config.PropertyA = "Value";
...

var unitOfWork = new UnitOfWorkA(config, dependenyA, ....);

However, there is still the problem with the dependencies that must be injected manually. Depending on the DI Framework, you can use a factory for creating the UnitOfWorks with all it's dependencies. Ninject for example has an excellent Extensions providing exactly that by creating a simple interface:

public interface IUnitOfWorkFactory
{
    UnitOfWorkA CreateUnitOfWorkA(ConfigurationA config);
    UnitOfWorkB CreateUnitOfWorkB(ConfigurationB config);
    ....
}

Just register the interface as factory and you can use it for creating the unit of work objects. If the unit of work has additional constructor dependencies (which have to be registered in the DI container of course), Ninject injects these dependencies automatically.

That solution looks a) simpler with b) less code and c) without using a service locator ;)

\$\endgroup\$
  • \$\begingroup\$ but I would need to write a factory method for every unit of work right? so I exchange the reflection and the service locator with an factory. Its just less flexible^^ \$\endgroup\$ – NtFreX Apr 27 '17 at 20:59
  • \$\begingroup\$ and the builder pattern can't be dropped.. it's needed for all the default values and different configurations \$\endgroup\$ – NtFreX Apr 27 '17 at 21:00
  • \$\begingroup\$ You don't need to implement the interface - just define it and Ninject creates it's implementation "magically". \$\endgroup\$ – JanDotNet Apr 27 '17 at 21:03
  • \$\begingroup\$ that sounds not to bad \$\endgroup\$ – NtFreX Apr 27 '17 at 21:04
  • \$\begingroup\$ but I'm using PRISM.. which doesn't want to relay on a specific dependency injection implementation.. as far as I remember \$\endgroup\$ – NtFreX Apr 27 '17 at 21:05
0
\$\begingroup\$

I am unable to leave comments due to the amount of reputation I have, so I have to resort to leaving a post.

Is what I do bad practice?

Yes. Service location is a well established anti-pattern. If you have to resort to using service location there is something wrong with the underlying design of your application.

\$\endgroup\$
  • 2
    \$\begingroup\$ I humbly recommend you to provide a complete example of a reworked version (since I myself was unable to come up with one that is less complex). It's not really helpful in this specific case to know it's a bad practice without knowing what the best practice would look like. \$\endgroup\$ – Igor Soloydenko Apr 27 '17 at 20:36

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.