Crooked architecture

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());
}

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.

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

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

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.

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

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

@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.

@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.

@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.

@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.

Point taken. You’re right. The masterswitch

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

@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.