Re-architecting a classic inheritance design

Question

I have the opportunity to rewrite a core piece of a project (C#) that is inheritance-heavy and feels increasingly restrictive in how it is designed. The scenario is pretty simple, imagine an application that is built to handle various small tasks. Every task is unique in it's functionality so it implements an ITask interface with a few simple methods that get implemented by each task class (e.g. Init(), Start(), Stop(), etc).

In addition to the ITask interface there is also an abstract BaseTask class that all tasks inherit from. This BaseTask class is sort of an 'everything but the kitchen sink' approach that contains useful pieces of functionality that the tasks often require. For example, all tasks record their status and progress the exact same way so this logic is defined in the BaseTask and available to all children through a protected method. Or to give another example, some tasks have to send email notifications out when certain conditions are met, so again, the BaseTask provides access to the core smtp functionality of the app for this purpose. There are many examples of these kind of 'helper' methods/properties

The problem of course is that this base class has already grown unwieldy and cluttered with properties and methods that are not universal to all tasks and it's starting to introduce more problems than it is solving. For example, anytime a change is required to the base class you find yourself in a position where you might wind up breaking some (or all) of the tasks. It also just seems like its breaking encapsulation and the single responsibility principal. It's also quite clear how this happened in the first place (and a common occurrence too). Initially this was a very small, specialized application but it blew up in terms of scope and size very quickly and what was initially probably not a bad decision architecturally has proven inflexible and awkward.

Where would you start with something like this? How would you approach the problem and what principals or design patters would you choose? Thank you very much

Answer 1

Step 1: Make each of those methods in BaseTask static and pass any required private members as arguments. Now they can live, and breathe comfortably, outside of the class.

Step 2: Implement an IOC container. Resolve your tasks from there.

Step 3: Take all of those protected methods and move them to a service, make them public. Make them non-static. Don't use them yet.

Step 4: Find a related group of methods, delete them and create an interface that exposes all of those methods. Implement that interface on your service (should be easy, as you're deleting methods that exist here) and constructor-inject them into only the tasks that are no longer compiling.

Step 5: Repeat step 4 until all the methods have gone from BaseTask.

Step 6: Break the service up into smaller services that implement only one interface; let your IOC registration know.

Step 7: Refactor til you drop.

Answer 2

This sounds like a prime candidate for the SOLID principles.

S: Single Responsibility Principle

Separate out all the functionality in the base class into separate classes, each doing only one thing. Need to send emails through an SMTP server? Build a single class that does this. Need to send emails through an Exchange server (without using its SMTP interface)? Implement a different class that does this. If needs be, create inheritance here with what is common.

For instance, sending email is pretty much universal in requirements, so build a base class that has the common items, and derived classes that implement the actual sending logic, ie. talking to the different kinds of servers.

O: Open/Closed Princple

Instead of making one task able to report through email, make it able to report through some common interface that can take the type of report it produces. If it reports only dumb text, that's what the interface should be about. Don't make the interface named or topicly related to email. This way you can bolt-on new classes later that can take those reports and send them through IM, through SMS, dump them to a logging system, or similar.

If the task needs input, say from a file, make it instead take an interface akin to a "input source" which it would ask for the input to process. This way the task could take any kind of source as input, as long as the source could produce the input in the format required by the task. Today you can process files, tomorrow you can process records from a database, or pages downloaded over HTTP.

This way you can finalize the logic of the task (close it), while still keep it open for new features later on (new types implementing the required interfaces).

L: Liskov Substitution Principle

(not sure how this would apply)

I: Interface Segregation Principle

Move everything out of the base class, except those things that logically belong in there, ie. those things that all tasks require/need. Everything else should be in small classes that can easily be tested by themselves, reused in other places, etc.

D: Dependency Inversion Principle

Instead of adding SMTP capabilities to a task that needs to report progress, make it take in an object implementing a generic (not in the .NET/C# way, but a general-purpose) interface that is about reporting, and make the task type use that object for its reporting purposes. This way, today, you only have email reporting using SMTP, but the knowledge about how to send that report is neither built into the class, or hard-wired into the environment of the class. The task can in the future be asked to report over any kind of medium that can be targetted by a type implementing that very same interface.

Answer 3

I've seen the same complexity in Infragistics controls which can do just about anything you want but they provide an unwieldy API that is almost impossible to navigate without experience.

With large APIs it makes sense to logically group methods into functional suites that all serve a single area of responsibility. For this I might create decorators. Doing so is very simple and straightforward so it will greatly reduce your chances of introducing bugs. Each decorator is instantiated as needed in order to access the features it provides. Your decorator would do nothing more than wrap the core object (a task) with additional properties/methods -- relocating the existing ones from the core object to the decorator object.

Answer 4

Do like @pdr suggested, it is a good solution. To avoid the interface hell mentioned in a comment, consider to use the Observer pattern. That way, you can add plugins with new functionality at any time without changing any existing (task) code.

Answer 5

I think this huge base class is a symptom of something deeper.

In your case I see two problems, which you should focus on solving before you start refactoring.

1. Developers are not experienced in OOP design. This is quite common problem. Instead of creating proper OOP structure, they simply find one global place and put their code here. So even if you have some semblance of OOP on the outside, it degenerates into procedural code on the inside. Try teach your developers in proper OOP design and motivate them to practice it.
2. The Task abstraction stopped being enough. Thanks to much complex requirements, it seems that you should make much more fine-grained abstraction. So instead of focusing on implementing specific task, you focus on separate components and then compose final task from those components. If you had proper OOP design and were doing refactoring as you went, you would probably see this problem much sooner.

If you don't fix these problems before you start refactoring, the problem might reappear. But this time, it will be even worse thanks to increased complexity.

Also, I would recomend you do practice Test Driven Development or at least make unit tests for most common use-cases. This will help you understand how your design should look like from outsider's perspective.

Answer 6

You may want some intermediate classes. Say classes W and X want to do something one way, and classes Y and Z want to do it another. You currently have some confusing method in base class A that does it both ways with lots of if's. Create subclasses of A, say B and C, each with its own copy of this method containing only relevant code. Now make W and X inherit from B and Y and Z inherit from C.

This gets complicated as you add methods, but with luck and a little brilliance you have a chance of ending up with orderly, easy-to-understand, simple, DRY code. There may be lots of classes and several levels of inheritance, but each class ought to be easy to understand.