Diving into TDD — Take 3

[Note: I’m using Copy as HTML to transfer code from VS.Net to BlogJet. This tool embeds style sheets into the posted blog entry to provide the nice formatting for the source code. Unfortunately, the version of .Text used by Dot Net Junkies won’t let me post the embedded style sheets. This means that viewing this article in an aggregator will look different, and worse, than viewing it on the DotNetJunkies site. I recommend viewing it there, as the code will be more easily readable than in an aggregator — bab]

Well, we’ve written two tests so far, and we’ve refactored this thing to within an inch of its life. We have a bit more to go before we get the Payroll portion of this first story finished up, after which we have to deal with reading the input batch file and writing the output check list, and also write a main. I’m much less concerned about those functions, because I know I can handle them. They’re part of the plumbing that makes the program work, rather than the meat of the program, so I generally leave those kinds of things for last.

As of now, we can pay one person, as long as it is always the first of the month. We should probably expand on both of those things, so that we can pay people or not, based on the day of the month, and we should also try to pay more than one person. Going back to our test list, those are the two remaining tests for this class.

Paying multiple employees

I’m going to choose to implement paying multiple people next. As a simplifying assumption, I’m always going to pass in a valid pay date when calling Payroll.Pay, since I haven’t implemented the logic to make paydays conditional on date yet. This is a pretty standard trick when implementing a system using TDD — You know you have some complex conditional behavior to be implemented soon, but you haven’t implemented it yet. What you can do is to write your tests now to reflect that condition always being true. This lets you make progress now, and also provides a safety net to prove that your code still works for the true case later, when you do implement the conditional.

So, as our first step, we need to write a test that tries to pay multiple people. That test might look like this:

     1: [Test]
     2: public void PayAllEmployeesOnFirstOfMonth()
     3: {
     4:     EmployeeList employees = new EmployeeList();
     5:     employees.Add("Bill", 1200);
     6:     employees.Add("Tom", 2400);
     7:  
     8:     Payroll payroll = new Payroll(employees);
     9:  
    10:     IList payRecords = payroll.PayAll("05/01/2004");
    11:  
    12:     Assert.AreEqual(2, payRecords.Count);
    13:  
    14:     PayRecord billsPay = payRecords[0] as PayRecord;
    15:     Assert.AreEqual("Bill", billsPay.Name);
    16:     Assert.AreEqual(100, billsPay.Pay);
    17:  
    18:     PayRecord tomsPay = payRecords[1] as PayRecord;
    19:     Assert.AreEqual("Tom", tomsPay.Name);
    20:     Assert.AreEqual(200, tomsPay.Pay);
    21: }

It is very similar to the previous tests. The only real differences are that we add two employees to the EmployeeList, and we have a new method, seen on Line 10, called Payroll.PayAll. This is a little tool-centric and language-imposed trick I’m using here. The existing Payroll.Pay method takes a single date and returns a single PayRecord. In an ideal world, I’d really like to just change that method to return an IList containing PayRecords, but language rules don’t let us override a method purely on return type. So, to have another Pay-like method that returns an IList of PayRecords, I have to have a differently named method.

I do have other options, rather than creating this newly named method.

  1. I could write the test to use the IList-returning Payroll.Pay method. This is where I eventually intend to be, after all. My objection to this is that it would require me to change multiple tests all at the same time to get this to compile and work. That’s nowhere near the smallest step I could make, and I’d rather find another path.

  2. I could comment out this newest test and refactor the previous tests to use the new version of the Payroll.Pay method. To do this the right way, I’d use a ChangeMethodSignature refactoring, which would involve these steps:

  3. Create new method with signature I want.

  4. Make the old method call the new method

  5. Retarget all calls to the old method to call the new one

The problem with this is that to do step 1, we’d have to override the existing method with a new method differing only in return type, getting us back to right where we are now with the PayAll method, at least temporarily.

  1. We could proceed as we are now, make PayAll work, then convert all existing tests to call PayAll, one at a time. Once that is finished, no one should be using the existing Pay method, so we can get rid of it, and rename the PayAll method back to Pay, as we wanted. The reason I’ll choose this path is that my toolset (ReSharper) is really good at things like RenameMethod, so I can do that easily.

So at this point, I’ll go ahead and make the above test work as written. The first thing I need to do is to expand the EmployeeList class to actually have a list of employees, instead of just one. This is a simple refactoring, involving replacing the scalar Employee object with an ArrayList of employees, and adapting the existing methods to use the first item in the ArrayList as the single employee previously defined in the class. After doing this, I reran all my tests to confirm that all tests other than the one I’m implementing now still work, and they did. Here is the new code, along with a new method, Employees, that returns the entire list of employees:

     1: public class EmployeeList
     2: {
     3:     private ArrayList employees = new ArrayList();
     4:  
     5:     public void Add(string employeeName, int yearlySalary)
     6:     {
     7:         employees.Add(new Employee(employeeName, yearlySalary));
     8:     }
     9:  
    10:     public int Count
    11:     {
    12:         get { return employees.Count; }
    13:     }
    14:  
    15:     public Employee Employee
    16:     {
    17:         get { return employees[0] as Employee; }
    18:     }
    19:  
    20:     public IList Employees
    21:     {
    22:         get { return employees; }
    23:     }
    24: } 

Now the Payroll.PayAll method:

     1: public IList PayAll(string payDate)
     2: {
     3:     ArrayList payRecords = new ArrayList();
     4:     foreach(Employee employee in employees.Employees)
     5:     {
     6:         payRecords.Add(employee.Pay(payDate));
     7:     }
     8:  
     9:     return payRecords;
    10: }

All tests work at this point, so its time for some refactoring. Let’s start by changing the first test to use the new PayAll method:

     1: [Test]
     2: public void NoOnePaidIfNoEmployees()
     3: {
     4:     EmployeeList employees = new EmployeeList();
     5:     Payroll payroll = new Payroll(employees);
     6:  
     7:     IList payrollOutput = payroll.PayAll("05/01/2004");
     8:  
     9:     Assert.AreEqual(0, payrollOutput.Count);
    10: }

And now, the last test to change:

     1: [Test]
     2: public void PayOneEmployeeOnFirstOfMonth()
     3: {
     4:     EmployeeList employees = new EmployeeList();
     5:     employees.Add("Bill", 1200);
     6:  
     7:     Payroll payroll = new Payroll(employees);
     8:  
     9:     IList payrollOutput = payroll.PayAll("05/01/2004");
    10:  
    11:     Assert.AreEqual(1, payrollOutput.Count);
    12:             
    13:     PayRecord billsPay = payrollOutput[0] as PayRecord;
    14:     Assert.AreEqual("Bill", billsPay.Name);
    15:     Assert.AreEqual(100, billsPay.Pay);
    16: }

And now, the old Pay method is no longer used, so I can go ahead and get rid of it, followed by a RenameMethod on PayAll to Pay, and we’re right where we wanted to be.

What if it’s not the first of the month?

Our original spec said that we shouldn’t pay an employee if payroll gets run on any day but the first of the month. We’ve avoided coding that so far, but we kind of need that to complete the Payroll class. So, let’s write a test. First question is, now that we’re thinking about day of the month, do we need to write any tests to confirm that we can pay on the first? Well, all the tests that we’ve written previously have assumed that the date was the first of the month, so each of those test cases should still function. That’s our proof that we can pay if we need to. Now we need to not pay if we don’t need to. Here is that test:

     1: [Test]
     2: public void NoEmployeePaidIfNotFirstOfMonth()
     3: {
     4:     EmployeeList employees = new EmployeeList();
     5:     employees.Add("Johnny", 3600);
     6:  
     7:     Payroll payroll = new Payroll(employees);
     8:     IList emptyList = payroll.Pay("05/02/2004");
     9:  
    10:     Assert.AreEqual(0, emptyList.Count);
    11: }

Run it, watch it fail, and now implement this functionality. Now the question is where should it go? Here is one implementation of it:

     1: public IList Pay(string payDate)
     2: {
     3:     ArrayList payRecords = new ArrayList();
     4:     foreach(Employee employee in employees.Employees)
     5:     {
     6:         if(employee.IsPayday(payDate))
     7:         {
     8:             payRecords.Add(employee.Pay());                  
     9:         }
    10:     }
    11:  
    12:     return payRecords;
    13: }

This would certainly work, and it is a solution I see a lot, but there is a philosophical issue with it. There is this OO design principle called Tell, Don’t Ask. What it says is that you shouldn’t ask an object a question about its state and then do something based on the response. If you think about it, that’s really exposing something about the internals of the object you’re talking to. The whole point of objects is that they are supposed to encapsulate data and behavior. If we let them do this, we end up with an interface that is made up of interesting methods, each of which has some meaning and context. If we don’t follow this rule, we end up with a lot of dumb data classes and a few god classes in our system that hold all the interesting behavior. I say, move the behavior and data to the same place, and get rid of simple accessor/setter methods!

To implement this change following Tell, Don’t Ask required a few more changes than the original change. First of all, the Employee.Pay method is changed:

     1: public PayRecord Pay(string payDate)
     2: {
     3:     if(IsPayday(payDate))
     4:     {
     5:         PayRecord payRecord = new PayRecord(Name, CalculatePay());
     6:         return payRecord;
     7:     }
     8:     else return null;
     9: }

And the Payroll.Pay method has to be changed as well. We need to look out for the null PayRecord coming back from the Employee.Pay method, and only add the returned PayRecord to our collection of PayRecords if it is not null. I’m not so sure this code looks any better than the first attempt we made by exposing the Employee.IsPayday method as public… 

The real solution to this dilemma is to use another pattern, called a CollectingParameter, to get rid of the null checking. Basically, if you hold your head just right, calling a method on another object and getting back a value, and doing something different based on whether that value is null or not, is really kind of a Tell, Don’t Ask violation. The same logic that is being done in the calling method through a check for null could realy be moved into the called method, provided the called method has someplace to put the result. That someplace is the CollectingParameter. This is how the refactoring turns out…

Inside Payroll.Pay, we get back to a very simple interface, with no extra state-based code:

     1: public IList Pay(string payDate)
     2: {
     3:     ArrayList payRecords = new ArrayList();
     4:     foreach(Employee employee in employees.Employees)
     5:     {
     6:         employee.Pay(payDate, payRecords);
     7:     }
     8:  
     9:     return payRecords;
    10: }

Note how we create our CollectingParameter payRecords at the top of the Pay method and pass it into the Employee.Pay method each time it is called. This allows the logic about whether or not to add a PayRecord into the Employee class, which is where the behavior that controls Pay logic lives. This is a better distribution of responsibilities. Here is the Employee.Pay code:

     1: public void Pay(string payDate, IList payRecords)
     2: {
     3:     if(IsPayday(payDate))
     4:     {
     5:         PayRecord payRecord = new PayRecord(Name, CalculatePay());
     6:         payRecords.Add(payRecord);
     7:     }
     8: }

And finally, here is all the code, as far as we’ve implemented so far. We’re still left with reading and writing to and from our input and output files, but we’ll cover that next time. There are some interesting design decisions in there that could spark some good conversation (if anyone is actually reading this!)

     1: using System.Collections;
     2: using NUnit.Framework;
     3:  
     4: namespace PayrollExercise
     5: {
     6:     [TestFixture]
     7:     public class PayrollFixture
     8:     {
     9:         [Test]
    10:         public void NoOnePaidIfNoEmployees()
    11:         {
    12:             EmployeeList employees = new EmployeeList();
    13:             Payroll payroll = new Payroll(employees);
    14:  
    15:             IList payrollOutput = payroll.Pay("05/01/2004");
    16:  
    17:             Assert.AreEqual(0, payrollOutput.Count);
    18:         }
    19:  
    20:         [Test]
    21:         public void PayOneEmployeeOnFirstOfMonth()
    22:         {
    23:             EmployeeList employees = new EmployeeList();
    24:             employees.Add("Bill", 1200);
    25:  
    26:             Payroll payroll = new Payroll(employees);
    27:  
    28:             IList payrollOutput = payroll.Pay("05/01/2004");
    29:  
    30:             Assert.AreEqual(1, payrollOutput.Count);
    31:             
    32:             PayRecord billsPay = payrollOutput[0] as PayRecord;
    33:             Assert.AreEqual("Bill", billsPay.Name);
    34:             Assert.AreEqual(100, billsPay.Pay);
    35:         }
    36:  
    37:         [Test]
    38:         public void PayAllEmployeesOnFirstOfMonth()
    39:         {
    40:             EmployeeList employees = new EmployeeList();
    41:             employees.Add("Bill", 1200);
    42:             employees.Add("Tom", 2400);
    43:  
    44:             Payroll payroll = new Payroll(employees);
    45:  
    46:             IList payRecords = payroll.Pay("05/01/2004");
    47:  
    48:             Assert.AreEqual(2, payRecords.Count);
    49:  
    50:             PayRecord billsPay = payRecords[0] as PayRecord;
    51:             Assert.AreEqual("Bill", billsPay.Name);
    52:             Assert.AreEqual(100, billsPay.Pay);
    53:  
    54:             PayRecord tomsPay = payRecords[1] as PayRecord;
    55:             Assert.AreEqual("Tom", tomsPay.Name);
    56:             Assert.AreEqual(200, tomsPay.Pay);
    57:         }
    58:  
    59:         [Test]
    60:         public void NoEmployeePaidIfNotFirstOfMonth()
    61:         {
    62:             EmployeeList employees = new EmployeeList();
    63:             employees.Add("Johnny", 3600);
    64:  
    65:             Payroll payroll = new Payroll(employees);
    66:             IList emptyList = payroll.Pay("05/02/2004");
    67:  
    68:             Assert.AreEqual(0, emptyList.Count);
    69:         }
    70:     }
    71:  
    72:     public class Payroll
    73:     {
    74:         private EmployeeList employees;
    75:  
    76:         public Payroll(EmployeeList employees)
    77:         {
    78:             this.employees = employees;
    79:         }
    80:  
    81:         public IList Pay(string payDate)
    82:         {
    83:             ArrayList payRecords = new ArrayList();
    84:             foreach(Employee employee in employees.Employees)
    85:             {
    86:                 employee.Pay(payDate, payRecords);
    87:             }
    88:  
    89:             return payRecords;
    90:         }
    91:     }
    92:  
    93:     public class Employee
    94:     {
    95:         private string name;
    96:         private int salary;
    97:  
    98:         public Employee(string name, int yearlySalary)
    99:         {
   100:             this.name = name;
   101:             this.salary = yearlySalary;
   102:         }
   103:  
   104:         private string Name
   105:         {
   106:             get { return name; }
   107:         }
   108:  
   109:         private int CalculatePay()
   110:         {
   111:             return salary/12;
   112:         }
   113:  
   114:         public void Pay(string payDate, IList payRecords)
   115:         {
   116:             if(IsPayday(payDate))
   117:             {
   118:                 PayRecord payRecord = new PayRecord(Name, CalculatePay());
   119:                 payRecords.Add(payRecord);
   120:             }
   121:         }
   122:  
   123:         private bool IsPayday(string payDate)
   124:         {
   125:             string [] dateParts = payDate.Split(new char[] {'/'});
   126:             return dateParts[1] == "01";
   127:         }
   128:     }
   129:  
   130:     public class PayRecord
   131:     {
   132:         private string name;
   133:         private int pay;
   134:  
   135:         public PayRecord(string name, int pay)
   136:         {
   137:             this.name = name;
   138:             this.pay = pay;
   139:         }
   140:  
   141:         public string Name
   142:         {
   143:             get { return name; }
   144:         }
   145:  
   146:         public int Pay
   147:         {
   148:             get { return pay; }
   149:         }
   150:     }
   151:  
   152:     public class EmployeeList
   153:     {
   154:         private ArrayList employees = new ArrayList();
   155:  
   156:         public void Add(string employeeName, int yearlySalary)
   157:         {
   158:             employees.Add(new Employee(employeeName, yearlySalary));
   159:         }
   160:  
   161:         public int Count
   162:         {
   163:             get { return employees.Count; }
   164:         }
   165:  
   166:         public Employee Employee
   167:         {
   168:             get { return employees[0] as Employee; }
   169:         }
   170:  
   171:         public IList Employees
   172:         {
   173:             get { return employees; }
   174:         }
   175:     }
   176: }