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?

No correct solution

Licensed under: CC-BY-SA with attribution
scroll top