Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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

share|improve this question
5  
In the future, please do not cross-post questions between Stack Exchange sites. If it is a better fit on a different site, please flag it for moderator review instead. Thanks. –  Thomas Owens May 17 '13 at 18:38
add comment

6 Answers

up vote 8 down vote accepted

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.

share|improve this answer
    
Nice, I came to mention Step 4 but your answer says it all and more. Interfaces are the key to escaping inheritance hell. –  Evan Plaice May 17 '13 at 18:52
1  
@EvanPlaice But then, you get into interface hell, where you have dozens of interfaces. Quite often with single method and/or single implementation. Which in the end makes the design even harder to understand, navigate and modify. –  Euphoric May 17 '13 at 18:56
    
@Euphoric: The only time that becomes hell is if you don't bother to group related functionality. I've never actually seen the case where there are too many interfaces. I've seen this inheritance hell almost everywhere I've worked. –  pdr May 17 '13 at 19:04
    
@pdr Isn't that what namespaces are for? Defining everything in the global namespace is no different than creating mega-monolithic classes. While interfaces can make code organization messy if misused, it's a lot harder to code yourself into a corner the same way a lot of people manage to do with class inheritance. –  Evan Plaice May 17 '13 at 19:10
    
@pdr Thanks for the reply, this makes good sense as an approach to break out common functionality like this. What about properties that are exposed off the BaseTask class, should I be as concerned about those as I am the methods? For example, say every task has a property for representing the user who initiated the task (IUser), is it still appropriate to expose such a shared property from the base class or should those be refactored as well? –  bruiseruser May 17 '13 at 19:15
show 6 more comments

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.

share|improve this answer
add comment

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.

share|improve this answer
    
+1 for the decorators –  RMalke May 17 '13 at 19:27
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
    
And then when you need to implement something from B in Y? ... Sorry, but inheritance trees are the worst solution to this problem. –  pdr May 17 '13 at 19:26
    
If the child classes don't fall into related groups and subgroups then no, this doesn't work at all. I've found that with most real problems they usually do. When they don't, there's usually something wrong with the basic design--though a programmer often can do nothing about that. So yeah, a lot of times this won't work. But it's far and away the best solution if you can make it work. –  RalphChapin May 17 '13 at 19:58
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.