Refactoring - Consolidate Duplicate Conditional Fragments |   | This is one of the easiest Refactorings to employ but it can be quite helpful. The example I'm going to use is quite simplified, so much so that it's probably oversimplified. Basically we have a conditional tree and depending on the Employee's title, his/her eligibility is set and SendToHR is set passing in their respective eligibility. This is a very simple example but I've definitely seen many where code is copied and pasted and done a few times. Before you know it you have a good bit of code that's redundant. When it's redundant, you have to make changes to every instance that calls it. This is a maintenance nightmare and it tends to make things more difficult to read and obscures one's intent.
Fortunately, fixing it is one of the easiest things to implement:
First, note that I've added two new fields to the Employee class, Title and Eligible. This is a horrible implementation and it's intentional because I intend to introduce another refactoring based on it. Nonetheless, let's assume we're fixing things in an assembly line..so we fix all Problem x's before we move on to Problelm Y's.
Here's the Employee Class:
using System;
namespace RefactorSamples
{
/// <summary>
/// Summary description for Employee.
///
public class Employee
{
private Name empName;
private string title;
private bool eligible;
public Employee()
{
//
// TODO: Add constructor logic here
//
}
public Employee(string first, string last)
{
}
public Name EmployeeName
{
get{return empName;}
set{empName = value;}
}
public string Title
{
get{return title;}
set{title = value;}
}
public bool Eligible
{
get{return eligible;}
set{eligible = value;}
}
}
} |
Now, here's the usage:
private void btnConsolidate_Click(object sender, System.EventArgs e)
{
Employee emp = new Employee();
switch(emp.Title)
{
case "President":
emp.Eligible = true;
SendToHR(emp.Eligible);
SendToPayRoll(emp.Eligible);
break;
case "Vice President":
emp.Eligible = true;
SendToHR(emp.Eligible);
SendToPayRoll(emp.Eligible);
break;
default:
emp.Eligible = false;
SendToHR(emp.Eligible);
SendToPayRoll(emp.Eligible);
break;
}
}
private void SendToHR(bool eligible)
{
//Do Something
}
private void SendToPayRoll(bool eligible)
{
//Do Something
} |
Can you see the problem here? If we added 10 new classes of Title here, we'd have to add two lines of code for each one, the call to SendToHR and the call to SendToPayroll. In practice it's often a lot more pronounced. Also, if you don't do the refactor, you won't be able to just call those two functions after the conditional because you already are calling it in the first three cases. So more than likely you'll code in the calls in the new Titles. Actually, the right thing to do is break it out of these three, code the new ones correctly and be done with it.
private void btnConsolidate_Click(object sender, System.EventArgs e)
{
Employee emp = new Employee();
switch(emp.Title)
{
case "President":
emp.Eligible = true;
break;
case "Vice President":
emp.Eligible = true;
break;
default:
emp.Eligible = false;
break;
}
SendToHR(emp.Eligible);
SendToPayRoll(emp.Eligible);
} |
In this trivial example, we just cut out a total of 4 lines of code and made it much easier to maintain. We also don't have to worry about anyone thinking that our intent is different for each case or continuing with this error for the sake of consistency. Like I said, this is a very easy Refactoring to implement and it can make things tighter, easier to read and easier to maintain. Another thing you may want to note. What if the common code was at the beginning? You'd do the same thing, just move it above the swtich statement. Either way the same principle applies!
|
