Sponsored By Aspose - File Format APIs for .NET

Aspose are the market leader of .NET APIs for file business formats – natively work with DOCX, XLSX, PPT, PDF, MSG, MPP, images formats and many more!

Crooked architecture

By now I’ve got rid of most of the soot resulting from the reactions on a previous post. Flames were pointed at my need for a case selector. In this post I will delve a little deeper into that. I’m only presenting the core of the problem, the point where the case seems unavoidable and hoping for any more elegant solutions from you. I’m on a tightrope between an (over-)simplification of the problem, a strict NDA and you as a critical reader. Nevertheless I think the problem is worth some reflections so I am giving at a try.

The domain

The domain of our system handles the reselling of licenses. A subscription has a any number of running licenses, to which it’s not much more than a key and an expiration date. Which makes a simple model, a subscription has a list of licenses. The crooked part is (and should be) completely invisible to the subscription. It is where we buy these licenses, we have several suppliers for them. Each license supplier has a completely different way we should interact with them. For one we do the complete administration and send them an Excel sheet every three months. For another one they do all administration themselves, every detail is communicated over a web service. For yet another one there is another different scenario. The list of these suppliers will grow over time. Adding another supplier will require writing specific code, but this code should affect other parts of the system as little as possible.

Having data stored in different ways (db table, web service, spreadsheet) per entity is not that great a problem; there is a vertical split. The core of our problem is that we have the data of one entity stored in a variety of ways; we have a horizontal split.

Consequences for the implementation

Having such different ways to get to the data has the consequence that the internal behavior of the license in the domain depends on the supplier of the license. The external behavior of the license should be the same for all licenses. It is highly undesired for the subscription-customer entity to know where the licenses where bought. (That’s where the profits are made :))

To make things more complicated, implementing the behavior of some suppliers will result in dragging in all kinds of code which should be completely unknown to the domain model. All solutions you presented me, including the nice one using lambda’s, would result in just that. For instance: when it comes to the web service based supplier we need things like credentials which are stored in the db. To get those you need the repository. You don’t want the domain model to depend on the repository; that’s even technical impossible. As a repository depends on a domain model that would result in a cyclic dependency.

A factory as man in the middle

These are the main parts (assemblies) of the solution together with the parts they depend on

  • Domain model
  • Repositories (domain model)
  • Communication with supplier1 (domain model, repositories)
  • Communication with supplier2 (domain model, repositories)
  • UI (domain model, repositories)

As stated before the main problem is how to get the supplier specific data of a subscription into the UI (and other parts) without knowing anything about specific suppliers. Our solution is a subscription factory. Only this factory has knowledge of the specific suppliers. It has references to all underlying layers, that is the domain model, the repositories and all supplier services.

When creating a new subscription object it queries all suppliers for licenses

public static Subscription Subscription(int customerId)

{

    var repository = new SubscriptionRepository();

    var subscription = repository.Subscription(customerId);

    subscription.Licenses = new List<Licence>();

    subscription.Licenses.AddRange(LicenceProvider1.GetLicences(customerId));

    subscription.Licenses.AddRange(LicenceProvider2.GetLicences(customerId, “UserName”, “Password”));

    return subscription;

}

The repository provides the plain db data for a subscription. The LicenceProviderX classes handle all supplier specific things. One class will just dive into our own repositories, the other one dives into the web to talk to the suppliers web service. Adding a new supplier leads to adding another line here.

This factory has to provide post production services as well. To do something with a license, like renewing it, requires talking to the license’s supplier. It’s up to the factory to do the magic of finding out who the supplier was. We use a database Id which is mirrored in an enum in code. (This is the dreaded enum this quest started with)

internal enum SupplierIds

{

    Supplier1,

    OtherSupplier

}

A helper method FindSupplier takes a license and returns the enum member. Using this the factory has a member to renew the licenses in a subscription

public static void OneMonthMore(Subscription subscription)

{

    foreach (var license in subscription.Licenses)

    {

        SupplierIds supplierId = FindSupplier(license);

        switch (supplierId)

        {

            case SupplierIds.Supplier1:

                LicenceProvider1.RenewLicence(1);

                break;

            case SupplierIds.OtherSupplier:

                LicenceProvider2.RenewLicence(1, “UserName”, “Password”);

                break;

        }

    }

}

And there is the switch. And IMHO it is a justified case.

Winding down

I don’t think I have presented you anything exiting at all. Which is IMHO good, because we do have a complicated problem. This solution works well in two ways. It is pretty easy to understand how it works and it is pretty easy to maintain. Adding another supplier boils down to, having written all specific code for that supplier, to adding another case. I hope I have made my point that there is a case for that. No doubt there are more elegant solutions. Please strike your matches. And enlighten me.

This entry was posted in Uncategorized. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • http://codebetter.com/members/pvanooijen/default.aspx pvanooijen

    @Aaron. What a nice way to come home after a holiday :) Else I wouldn’t have waited 10 days to react.
    You perfectly understood the problem, your solution is quite elegant and is explained quite well.

    What fits in very well is that every LicenseProvider can decide whether it handles a Licence and the number of Providers handling a licence is not fixed. In our case this number can (at this moment,software does change) be smaller as well as greater than one.

    Great !
    Thanks a lot for your contribution.

  • Aaron D.

    My suggestion is to create a LicenseProvider composite class that takes a dependency on all your specific license providers. You can use StructureMap or some other IoC container that can inject an array of dependencies to get all your implementations of LicenseProviders. So this would be the Composite class.

    public class CompositeLicenseProvider : ILicenseProvider{
    private LicenseProvider _providers[];

    public CompositeLicenseProvider(ILicenseProvider _licenseProviders[]){
    _providers = _licenseProviders;
    }

    public void RenewLicense(License license){
    GetProvider(license).RenewLicense(license.Id);
    }

    public ILicenseProvider GetProvider(License license){
    foreach(var provider in _providers){
    if(provider.Handles(license)) return provider;
    }
    throw new LicenseProviderNotFoundException(license);
    }
    }

    Now that your composite class has an instance of all of your provider classes, you can delegate the actual work to the class that should handle the license given. The trick to removing the switch statement is to delegate the decision for handling the current license to each LicenseProvider. So your ILicenseProvider would need a method to decide whether or not it can handle the given license.

    public interface ILicenseProvider{
    bool Handles(License license);
    }

    Handles would be implemented like FindSuppliers except that each provider would have only the code needed to determine whether it should handle the given license. For instance maybe it says:

    public class SomeLicenseProvider : ILicenseProvider{
    public bool Handles(License license){
    if(license.SupplierName == “Some”) return true;

    return false;
    }
    }

    and another LicenseProvider may be:

    public class SingleClientLicenseProvider: ILicenseProvider{
    public bool Handles(License license){
    if(license.Client == “The Client I Handle”) return true;

    return false;
    }
    }

    This puts all the logic for deciding which license provider will handle which license in the place that makes the most sense (IMHO), the LicenseProvider itself. This also delegates the provider specific information to each provider. For example in the code in your post the RenewLicense method takes different arguments depending on the Supplier. This context specific information should be encapsulated in each LicenseProvider implementation (again IMHO). So all of your LicenseProviders can implement the same interface, and in StructureMap you can use AddAllTypesOf<> to get it to inject the array of ILicenseProviders in the CompositeLicenseProvider.

    Now your OneMonthMore method becomes:

    public static void OneMonthMore(Subscription subscription)
    {
        foreach (var license in subscription.Licenses)
        {
          _compositeProvider.RenewLicense(1);  
         }
    }

    This removes the switch statement and moves all the decision logic about which provider handles which license type into each provider itself, fully encapsulating that decision process inside the only class that needs to know, the providers themselves. As a side benefit, if you have a new License Provider come along (I know that won’t happen, software never changes 😉 then you don’t need to update you Subscription object (and more importantly the next person who comes along won’t need to know to update it) because now it will look like this :

    public static Subscription Subscription(int customerId)
    {
        var repository = new SubscriptionRepository();
        var subscription = repository.Subscription(customerId);
        subscription.Licenses = new List ();
    subscription.Licenses.AddRange(_container.GetInstance().GetLicenses(customerId));
        return subscription;
    }

    Whenever a new LicenseProvider comes along you just need to create a new class for that specific provider and implement ILicenseProvider on it. Your IoC Container will automatically inject that new class into your composite LicenseProvider class. No need to search through the code and update anything, and no more switch statement.

    Yay!!

    Hopefully I didn’t completely misunderstand your problem, and if so thanks for reading anyway :)

  • http://codebetter.com/members/pvanooijen/default.aspx pvanooijen

    Point taken. You’re right. The masterswitch :)

  • Remco Schoeman

    @pvanooijen

    Ha, I got you there! There is no switch in my Suppliers class, but a fully armed and operational battlestation!

    ahem..

    What i meant was, if you can keep the switching hidden in a single class,and can prevent it from growing al over your code like mold, then a switch is nice and simple.

    Don’t fix it if it ain’t broken, I say.

  • http://codebetter.com/members/pvanooijen/default.aspx pvanooijen

    @Jakub

    I think I get the idea. When I compare my case with yours the difference is that my cases have quite different signatures. Which, as I understand you, does not work with a call registry.

  • http://codebetter.com/members/pvanooijen/default.aspx pvanooijen

    @Liam

    That does make the code better to read. The downside is that to build a list of licenceproviders requires instantiang objects of each of them. Something you do not want to do without the certainty the licenceprovider is going to be used. Specially for the providers which are reached over a web service.

  • http://codebetter.com/members/pvanooijen/default.aspx pvanooijen

    @Remco

    Your approach comes closer to our real project than my example does. Our suppliers implement one or more interfaces, one of them is ILicenceprovider. I left that layer out for the case of (over-)simplification

    In your code the switch has moved out of the example, but it is still there. It will be in Suppliers.GetLicenceProvider, that would be something like

    switch (supplierId)

           {

               case SupplierIds.Supplier1:

                   return new LicenceProvider1();

               case SupplierIds.OtherSupplier:

                   return new LicenceProvider2(“UserName”, “Password”);

             }

    Your code has three black boxes: FindSupplier, GetLicenceProvider and GetLicenceProviders.

    It is a good next step, but does IMHO not change the idea.

  • http://blog.gutek.pl/ Jakub Gutkowski

    What about completely different approach? Instead of Switch use something like call registry which allows you to have configuration in one place, and by this you are able to easily manage change in code that can occur in time.

    Here is a post I made some time ago about this:
    http://blog.gutek.pl/post/2010/05/10/Switch-inaczej.aspx

    it’s in PL, but you can use bing to translate it to English:
    http://www.microsofttranslator.com/bv.aspx?ref=Internal&from=pl&to=en&a=http%3a%2f%2fblog.gutek.pl%2fpost%2f2010%2f05%2f10%2fSwitch-inaczej.aspx

    it’s not the best translation, but should be fine :)

  • http://hackingon.net Liam McLennan

    You can generally replace a switch with an associative array. See http://hackingon.net/post/Unnamed-Refactoring.aspx

  • Remco Schoeman

    I’d do it like this:
    public static void OneMonthMore(Subscription subscription)
    {
    foreach (var license in subscription.Licenses)
    {
    SupplierIds supplierId = FindSupplier(license);
    ILicenceProvider provider = Suppliers.GetLicenceProvider(supplierId);
    provider.OneMonthMore(license);
    }
    }

    and

    public static Subscription Subscription(int customerId)
    {
    var repository = new SubscriptionRepository();
    var subscription = repository.Subscription(customerId);
    subscription.Licenses = new List ();

    foreach(ILicenceProvider provider in Suppliers.GetLicenseProviders())
    subscription.Licenses. AddRange(provider.GetLicences(customerId));
    return subscription;
    }

    The idea being that you create a factory for supplier specific behavior instead of a factory that behaves differently for each supplier.

    The refactoring from your example to mine can be done in small steps. Make the implemention lookup the default case in the switch, and for each supplier you can now create an ILicenseProvider implementation and remove that suppliers case from the switch statement.

    Supplier specific things for licence handling are now in implementations of ILicenceProvider (instead of in the Subscription factory).

    This gives the opportunity to create Supplier1 Supplier2,etc.. classes that implements ILicenseProvider, and you can go and find more methods with switches that do supplier specific behavior, and do the same trick until all the supplier specific code is in the SupplierX classes.
    But that depends on if you want a SupplierX class or just a SupplierXLicenseProvider.

  • http://codebetter.com/members/pvanooijen/default.aspx pvanooijen

    I do. But that logic is locked up inside the factory and not allowed to escape :) It’s the FindSupplier (private) method.

  • Tom de Koning

    Well, you must have some logic to determine the supplierId based on a license. In case of all suppliers being equal I would have the supplier resolved using MEF where some info in the license would determine the most likely supplier.

    I just don’t like / find it ugly to use switch :-)

  • http://codebetter.com/members/pvanooijen/default.aspx pvanooijen

    Nice, but…

    This requires subclassing the License, a new class for every supplier. There is on itself nothing wrong with subclassing but here it would lead to leaking the difference between licenses out of the factory straight into the domain model. The whole point was that all licenses are equal.

  • Tom de Koning

    Well, I still don’t like the switch statement. I avoid these constructs by resolving the required implementation based on the type of the request (here, the licence type).
    So in the BootStrapper we register all handlers using a marker interface (the licence type).

    Nuff said, you can do it like this:
    First declare your handler
    public class VerizonLicenceHandler : LicenceHandler

    have your handler resolved using
    Type type = GetHandlerFor(licence);
    var handler = (ILicenceHandler)container.Resolve(type);

    next you can easily do handler.Renew(licence);

    where:
    static Type GetHandlerFor(Licence licence)
    {
    return typeof(ILicenceHandler<>).MakeGenericType(licence.GetType());
    }