Self-Improvement Refactoring – How to Refactor Code to Some Common Code?

maintainabilityrefactoringself-improvement

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?

Best Answer

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.

Related Topic