TDD Dive — Part Deux

When last I left you, we had just about started implementing a simple payroll system using TDD. We had written two tests and done some refactoring, but there were still some code smells that I didn’t care for that we were going to look at before proceeding.

This is the current Payroll.Pay method:

     1: public string Pay(string payDate)
     2: {
     3:     if(employees.Count == 0) return null;
     4:     Employee theEmployee = employees.Employee;
     5:     return "Check,100," + theEmployee.Name + ",$" + theEmployee.CalculatePay() + "," +payDate;
     6: }

My biggest problems here are that this method, inside the Payroll class, knows how to dip inside the Employee class to get names and pay amounts, and it know how to format the output according to what our check writing company needs. Let’s try to fix both of these, and see where this takes us.

My first instinct is that we could move this behavior inside Employee. This would remove the knowledge of the primitive accessors from the Payroll class, and would move all logic about creating pay information inside the Employee. This leaves us with:

     1: public string Pay(string payDate)
     2: {
     3:     if(employees.Count == 0) return null;
     4:     Employee theEmployee = employees.Employee;
     5:     return Pay(payDate, theEmployee);
     6: }
     7:   
     8: private string Pay(string payDate, Employee theEmployee)
     9: {
    10:     return "Check,100," + theEmployee.Name + ",$" + theEmployee.CalculatePay() + "," +payDate;
    11: }

Step one was to do an ExtractMethod refactoring to move the creation of the string into its own method. Now that its here, we can look at it more closely, and see if we can’t figoure out where the code really wants to be. It sure looks like all the data that this method is using comes from the Employee class. This is a code smell called FeatureEnvy, where code that exists in one place is accessing private parts of another class, and the code really wants to be in that other class. Let’s do a MoveMethod and move it there…

     1: private class Payroll
     2: {
     3:     private EmployeeList employees;
     4:
     5:     public Payroll(EmployeeList employees)
     6:     {
     7:         this.employees = employees;
     8:     }
     9:  
    10:     public string Pay(string payDate)
    11:     {
    12:         if (employees.Count == 0) return null;
    13:         Employee theEmployee = employees.Employee;
    14:         return theEmployee.Pay(payDate);
    15:     }
    16: }
    17:  
    18: private class Employee
    19: {
    20:     private string name;
    21:     private int salary;
    22:  
    23:     public Employee(string name, int yearlySalary)
    24:     {
    25:         this.name = name;
    26:         this.salary = yearlySalary;
    27:     }
    28:  
    29:     private string Name
    30:     {
    31:         get { return name; }
    32:     }
    33:  
    34:     private int CalculatePay()
    35:     {
    36:         return salary/12;
    37:     }
    38:  
    39:     public string Pay(string payDate)
    40:     {
    41:         return "Check,100," + Name + ",$" + CalculatePay() + "," + payDate;
    42:     }
    43: }

After this change, line 14 has been changed to call the Pay method of Employee, and Payroll is ignorant of any other behavior or implementation detail of Employee. This is a Good Thing.

The remaining smell in our code at this point is that the Pay method now knows about check formatting, and it probably shouldn’t. What I’d really like to have happen is that this method would return some dumb data object that another class could use to format the output at some later point. That way I’ve separated production of the payroll data from the formatting and reporting of that data. Each of these concepts could vary independently, so they really don’t belong in the same place. This leads me to create a PayRecord class, whose responsibility it is to transport pay data between the Employee class and the still to-be-designed check formatting class.

     1: private class Employee
     2: {
     3:     public string Pay(string payDate)
     4:     {
     5:         PayRecord payRecord = new PayRecord(Name, CalculatePay());
     6:         return "Check,100," + payRecord.Name + ",$" + payRecord.Pay + "," + payDate;
     7:     }
     8: }
     9:  
    10: private class PayRecord
    11: {
    12:     private string name;
    13:     private int pay;
    14:  
    15:     public PayRecord(string name, int pay)
    16:     {
    17:         this.name = name;
    18:         this.pay = pay;
    19:     }
    20:  
    21:     public string Name
    22:     {
    23:         get { return name; }
    24:     }
    25:  
    26:     public int Pay
    27:     {
    28:         get { return pay; }
    29:     }
    30: }

Sometimes it gets challenging to imagine what the smallest step along the way could be. In this case, it would have been pretty easy to create a PayRecord class, change the signature of the Pay method to return one of these buggers, and then change my tests to match. But I think I can do it is a smaller step. I can do as I show above — I can create the PayRecord class inside the Pay method and still return the string. I can now test this,and I’m certain that my code all still works. The next refactoring is the ChangeMethodSignature, where I’ll change this method to return a PayRecord and change the tests to match. Remember, refactoring is about making small, measured changes to your source code, and verifying their correctness before moving on. Taking big steps increases the risk that something can go wrong, and you’ll have to take extra time to figure out what that was and fix it.

Now here is the final version of the code as far as we are now. This last step was to change the method signature of the Pay method in Employee and Payroll to both return a PayRecord, and I updated the tests to look inside the PayRecord to confirm that the right data was produced.

     1: using NUnit.Framework;
     2:  
     3: namespace PayrollExercise
     4: {
     5:     [TestFixture]
     6:     public class PayrollFixture
     7:     {
     8:         [Test]
     9:         public void NoOnePaidIfNoEmployees()
    10:         {
    11:             EmployeeList employees = new EmployeeList();
    12:             Payroll payroll = new Payroll(employees);
    13:  
    14:             PayRecord payrollOutput = payroll.Pay("05/01/2004");
    15:  
    16:             Assert.IsNull(payrollOutput);
    17:         }
    18:  
    19:         [Test]
    20:         public void PayOneEmployeeOnFirstOfMonth()
    21:         {
    22:             EmployeeList employees = new EmployeeList();
    23:             employees.Add("Bill", 1200);
    24:  
    25:             Payroll payroll = new Payroll(employees);
    26:  
    27:             PayRecord payrollOutput = payroll.Pay("05/01/2004");
    28:  
    29:             Assert.AreEqual("Bill", payrollOutput.Name);
    30:             Assert.AreEqual(100, payrollOutput.Pay);
    31:         }
    32:     }
    33:  
    34:     public class Payroll
    35:     {
    36:         private EmployeeList employees;
    37:  
    38:         public Payroll(EmployeeList employees)
    39:         {
    40:             this.employees = employees;
    41:         }
    42:  
    43:         public PayRecord Pay(string payDate)
    44:         {
    45:             if (employees.Count == 0) return null;
    46:             Employee theEmployee = employees.Employee;
    47:             return theEmployee.Pay(payDate);
    48:         }
    49:     }
    50:  
    51:     public class Employee
    52:     {
    53:         private string name;
    54:         private int salary;
    55:  
    56:         public Employee(string name, int yearlySalary)
    57:         {
    58:             this.name = name;
    59:             this.salary = yearlySalary;
    60:         }
    61:  
    62:         private string Name
    63:         {
    64:             get { return name; }
    65:         }
    66:  
    67:         private int CalculatePay()
    68:         {
    69:             return salary/12;
    70:         }
    71:  
    72:         public PayRecord Pay(string payDate)
    73:         {
    74:             PayRecord payRecord = new PayRecord(Name, CalculatePay());
    75:             return payRecord;
    76:         }
    77:     }
    78:  
    79:     public class PayRecord
    80:     {
    81:         private string name;
    82:         private int pay;
    83:  
    84:         public PayRecord(string name, int pay)
    85:         {
    86:             this.name = name;
    87:             this.pay = pay;
    88:         }
    89:  
    90:         public string Name
    91:         {
    92:             get { return name; }
    93:         }
    94:  
    95:         public int Pay
    96:         {
    97:             get { return pay; }
    98:         }
    99:     }
   100:  
   101:     public class EmployeeList
   102:     {
   103:         private int employeeCount = 0;
   104:         private Employee employee;
   105:  
   106:         public void Add(string employeeName, int yearlySalary)
   107:         {
   108:             employee = new Employee(employeeName, yearlySalary);
   109:             this.employeeCount++;
   110:         }
   111:  
   112:         public int Count
   113:         {
   114:             get { return employeeCount; }
   115:         }
   116:  
   117:         public Employee Employee
   118:         {
   119:             get { return employee; }
   120:         }
   121:     }
   122: }

The point that I really want to make is that refactoring is really important. Test Driven Development is the process of adding behavior incrementally, driven through mini-use cases of the software you’re building. Each test forces some new behavior to be added to the system, but it doesn’t say anything at all about the design of the system. The tests help you get the interface right between the objects you’re talking to, but that’s at far as they go. Refactoring is the design engine in the TDD process. It is the process through which other classes are born, code is made simple and readable, and systems evolve and grow.

If you are doing TDD and you are not spending a significant amount of your time refactoring, you are doing yourself and your team a disservice.

I’ll leave you on that cheery note 🙂 More of this example to come shortly.

— bab

Now playing: Dio – The Last In Line – One Night In The City

5 thoughts to “TDD Dive — Part Deux”

  1. Jeff,

    There is no final code yet. I’m doing this all in real-time as I write the blog entries. I’m doing this so that I can change the stories at will and see where it takes me.

    The code at the bottom of this article is the entire codebase as of now. You can also find the complete solution to this first story done in Java a few blog entries back from this one. The stories are identical, but the solutions differ somewhat, as they do every time.

    I appreciate the comment, and I hope you’ll stick around for the rest of the series as it gets posted. Tell your friends 🙂

    — bab

Leave a Reply

This site uses Akismet to reduce spam. Learn how your comment data is processed.