How to refactor code to some common code?

Question

Background

I'm working on an ongoing C# project. I'm not a C# programmer, primarily a C++ programmer. So I was assigned basically easy and refactoring tasks.

The code is a mess. It's a huge project. As our customer demanded frequent releases with new features and bug fixes, all other developers were forced to take brute force approach while coding. The code is highly unmaintainable and all other developers agree with it.

I'm not here to debate whether they did it right. As I'm refactoring, I'm wondering if I'm doing it in the right way as my refactored code seems complex! Here is my task as simple example.

Problem

There are six classes: A, B, C, D, E and F. All of the classes have a function ExecJob(). All six implementations are very similar. Basically, at first A::ExecJob() was written. Then a slightly different version was required which was implemented in B::ExecJob() by copy-paste-modification of A::ExecJob(). When another slightly different version was required, C::ExecJob() was written and so on. All six implementations have some common code, then some different lines of code, then again some common code and so on. Here is a simple example of the implementations:

A::ExecJob()
{
    S1;
    S2;
    S3;
    S4;
    S5;
}

B::ExecJob()
{
    S1;
    S3;
    S4;
    S5;
}

C::ExecJob()
{
    S1;
    S3;
    S4;
}

Where SN is a group of exact same statements.

To make them common, I've created another class and moved the common code in a function. Using parameter to control which group of statements should be executed:

Base::CommonTask(param)
{
    S1;
    if (param.s2) S2;
    S3;
    S4;
    if (param.s5) S5;
}

A::ExecJob() // A inherits Base
{
    param.s2 = true;
    param.s5 = true;
    CommonTask(param);
}

B::ExecJob() // B inherits Base
{
    param.s2 = false;
    param.s5 = true;
    CommonTask(param);
}

C::ExecJob() // C inherits Base
{
    param.s2 = false;
    param.s5 = false;
    CommonTask(param);
}

Note that, this example only employs three classes and oversimplified statements. In practice, the CommonTask() function looks very complex with all those parameter checking and there are many more statements. Also, in real code, there are several CommonTask()-looking functions.

Though all the implementations are sharing common code and ExecJob() functions are looking cuter, there exists two problems that are bothering me:

• For any change in CommonTask(), all six (and may be more in the future) features are needed to be tested.
• CommonTask() is already complex. It will get more complex over time.

Am I doing it in the right way?

Answer 1

Yes, you are absolutely in the right path!

In my experience, I noticed that when things are complicated, the changes happen in small steps. What you have done is the step 1 in the evolution process (or refactoring process). Here is step 2 and step 3:

Step 2

class Base {
  method ExecJob() {
    S1();
    S2();
    S3();
    S4();
    S5();
  }
  method S1() { //concrete implementation }
  method S3() { //concrete implementation }
  method S4() { //concrete implementation}
  abstract method S2();
  abstract method S5();
}

class A::Base {
  method S2() {//concrete implementation}
  method S5() {//concrete implementation}
}

class B::Base {
  method S2() { // empty implementation}
  method S5() {//concrete implementation}
}

class C::Base {
  method S2() { // empty implementation}
  method S5() { // empty implementation}
}

This is the 'Template Design Pattern' and it is one step ahead in the refactoring process. If the base class changes, the subclasses (A,B,C) don't need to get affected. You can add new subclasses relatively easily. However, right away from the picture above you can see that the abstraction is broken. The need for 'empty implementation' is a good indicator; it shows that there is something wrong with your abstraction. It might have been an acceptable solution for short-term, but there seems to be a better one.

Step 3

interface JobExecuter {
  void executeJob();
}
class A::JobExecuter {
  void executeJob(){
     helper = new Helper();
     helper->S1();
     helper->S2();
     helper->S3();
     helper->S4();
     helper->S5();
  }
}

class B::JobExecuter {
  void executeJob(){
     helper = new Helper();
     helper->S1();
     helper->S3();
     helper->S4();
     helper->S5();
  }
}

class C::JobExecuter {
  void executeJob(){
     helper = new Helper();
     helper->S1();
     helper->S3();
     helper->S4();
  }
}

class Base{
   void ExecJob(JobExecuter executer){
       executer->executeJob();
   }
}

class Helper{
    void S1(){//Implementation} 
    void S2(){//Implementation}
    void S3(){//Implementation}
    void S4(){//Implementation} 
    void S5(){//Implementation}
}

This is the 'Strategy Design Pattern' and seems as a good fit for your case. There are different strategies to execute the job and each class (A,B,C) implements it differently.

I am sure there is a step 4 or step 5 in this process or a lot better refactoring approaches. However, this one will let you eliminate duplicate code and make sure that the changes are localized.

Answer 2

You are actually doing the right thing. I say this because :

1. If you need to change the code for a common task functionality, you don't need to change it in all 6 classes which would contain the code if you don't write it in a common class.
2. Number of lines of code will be decreased.

Answer 3

You see this sort of code sharing a lot in with event driven design (.NET especially). The most maintainable way is to keep your shared behavior in as small chunks as possible.

Let the high level code reuse a bunch of small methods, leave the high level code out of the shared base.

You will have a lot of boiler plate in your leaf/concrete implementations. Don't panic, it's ok. All that code is direct, easy to understand. You'll have to rearrange it occasionally when stuff breaks, but it will be easy to change.

You'll see a lot of patterns in the high level code. Sometimes they are real, most of the time they aren't. The "configurations" of the five parameters up there look similar, but they aren't. They are three entirely different strategies.

Also want to note that you can do all this with composition and never worry about inheritance. You'll have less coupling.

Answer 4

If i was you i will probably add 1 more step at the beginning: a UML-based study.

Refactoring the code merging all the common parts together isn't always the best move, sounds more like a temporary solution than a good approach.

Draws a UML scheme, keep the things simple but effective, keep in mind some basic concepts about your project like "what is supposed to do this software?" "what is the best way to keep this piece of software abstract, modular, extensible, ... etc etc ?" "how i can implement the encapsulation at its best?"

I'm just saying this: do not care about the code right now, you only have to care about the logic, when you have a clear logic in mind all the rest can become a really easy task, in the end all the this kind of problems that you are facing is merely caused by a bad logic.

Answer 5

The very first step, no matter where this is going, should be breaking the apparently large method A::ExecJob into smaller pieces.

Therefore, instead of

A::ExecJob()
{
    S1; // many lines of code
    S2; // many lines of code
    S3; // many lines of code
    S4; // many lines of code
    S5; // many lines of code
}

you get

A::ExecJob()
{
    S1();
    S2();
    S3();
    S4();
    S5();
}

A:S1()
{
   // many lines of code
}

A:S2()
{
   // many lines of code
}

A:S3()
{
   // many lines of code
}

A:S4()
{
   // many lines of code
}

A:S5()
{
   // many lines of code
}

From here on, there are many possible ways to go. My take on it: Make A the base class of your class hierachy and ExecJob virtual and it becomes easy to create B, C, ... without too much copy-paste - just replace ExecJob (now a five-liner) with a modified version.

B::ExecJob()
{
    S1();
    S3();
    S4();
    S5();
}

But why have so many classes at all? Maybe you can replace them all with a single class that has a constructor which can be told which actions are necessary in ExecJob.

Answer 6

I agree with the other answers that your approach is along the right lines although I don't think that inheritance is the best way to implement common code - I prefer composition. From the C++ FAQ which can explain it far better than I ever could: http://www.parashift.com/c++-faq/priv-inherit-vs-compos.html

Answer 7

First, you should make sure that inheritance is really the right tool here for the job - only because you need a common place for functions used by your classes A to F does not mean that a common base class is the right thing here - sometimes a separate helper class does the job better. It may be, it may be not. That depends on having a "is-a" relationship between A to F and your common base class, impossible to say from artificial names A-F. Here you find a blog post dealing with this topic.

Let's assume you decide that the common base class is the right thing in your case. Then the second thing I would do is to make sure your code fragments S1 to S5 are each implemented in a separate methods S1() to S5() of your base class. Afterwards the "ExecJob" functions should look like this:

A::ExecJob()
{
    S1();
    S2();
    S3();
    S4();
    S5();
}

B::ExecJob()
{
    S1();
    S3();
    S4();
    S5();
}

C::ExecJob()
{
    S1();
    S3();
    S4();
}

As you see now, since S1 to S5 are just method calls, no code blocks any more, the code duplication has been almost completely removed, and you don't need any parameter checking any more, avoiding the problem of increasing complexity you might get otherwise.

Finally, but only as a third step(!), you might think about combining all those ExecJob methods into one of your base class, where the execution of those parts may be controlled by parameters, just the way you suggested it, or by using template method pattern. You have to decide yourself if that's worth the effort in your case, based on the real code.

But IMHO the basic technique to break down big methods into small methods is much, much more important for avoiding code duplication than applying patterns.