Tag Archives: Refactoring
Dependencies and Maintenance Cost

I was speaking with a colleague about the importance of reducing dependencies. I’ll reproduce here some work that we did to streamline some code and make it easier to test and maintain.

Consider this snippet as our starting point.

public Employee[] GetEmployees() {

    var url = MyApplication.Settings.RemoteApi.Url;
    var auth = new Auth(MyApplication.User);
    var remoteApi = new RemoteApi(auth, url);
    var rawData = remoteApi.FindEmployees();
    var mapper = new EmployeeMapper();
    var mappedResults= mapper.map(rawData);
    return mappedResults;
}

Dependencies

  1. MyApplication
  2. MyApplication.Settings
  3. MyApplication.Settings.RemoteApi
  4. MyApplication.Settings.RemoteApi.Url
  5. Auth
  6. User
  7. RemoteApi
  8. FindEmployees
  9. EmployeeMapper

That’s a lot objects or properties that will impact this code if they change for any reason. Further, when we attempt to write an automated test against this code we are dependent on a live existence of an API.

Law of Demeter

We can apply the Law of Demeter to reduce some of the dependencies in this code.

  • Each unit should have only limited knowledge about other units: only units "closely" related to the current unit.
  • Each unit should only talk to its friends; don’t talk to strangers.
  • Only talk to your immediate friends.

In practice this usually means replacing code that walks a dependency hiearchy with a well-named function on the hierarchy root and call the function instead. This allows you to change the underlying hierarchy without impacting the code that depends on the function.

Consider the following modifications.

public Employee[] GetEmployees() {

    var url = MyApplication.GetRemoteApiUrl();
    var auth = MyApplication.GetAuth();
    var remoteApi = new RemoteApi(auth, url);
    var rawData = remoteApi.FindEmployees();
    var mapper = new EmployeeMapper();
    var mappedResults= mapper.map(rawData);
    return mappedResults;
}

Dependencies

  1. MyApplication
  2. Auth
  3. User
  4. RemoteApi
  5. FindEmployees
  6. EmployeeMapper

I think we can do better.

public Employee[] GetEmployees() {

    var remoteApi = MyApplication.GetRemoteApi();
    var rawData = remoteApi.FindEmployees();
    var mapper = new EmployeeMapper();
    var mappedResults= mapper.map(rawData);
    return mappedResults;
}

Dependencies

  1. MyApplication
  2. RemoteApi
  3. FindEmployees
  4. EmployeeMapper

This is starting to look good. We’ve reduced the number of dependencies in this function by half by implementing a simple design change.

Dependency Inversion

We still have the problem that we are dependent on a live instance of the API when we write automated tests for this function. We can address this by applying the Dependency Inversion Principle

  • High-level modules should not depend on low-level modules. Both should depend on abstractions (e.g. interfaces).
  • Abstractions should not depend on details. Details (concrete implementations) should depend on abstractions.

In practice this means that our GetEmployees function should not depend on a concrete object. We can address this issue by passing the RemoteApi to the constructor of the class as opposed to having the method get the RemoteApi instance through MyApplication.


public class EmployeeRepository {

    private RemoteApi remoteApi;

    public EmployeeRepository(RemoteApi remoteApi) {
        this.remoteApi = remoteApi;
    }

    // snipped
    public Employee[] GetEmployees() {
        var rawData = this.remoteApi.FindEmployees();
        var mapper = new EmployeeMapper();
        var mappedResults= mapper.map(rawData);
        return mappedResults;
    }

Dependencies

  1. RemoteApi
  2. FindEmployees
  3. EmployeeMapper

With this change we have reduced dependencies even further. However, we have not satisifed the Dependency Inversion Principle yet because we are still dependent on a concrete class: RemoteApi. If we extract an interface from RemoteApi and instead depend on that then the DIP is satisfied.


public interface IRemoteApi {
    RawEmployeeData[] FindEmployees();
}

public class EmployeeRepository {

    private IRemoteApi remoteApi;

    public EmployeeRepository(IRemoteApi remoteApi) {
        this.remoteApi = remoteApi;
    }

    // snipped
    public Employee[] GetEmployees() {
        var rawData = this.remoteApi.FindEmployees();
        var mapper = new EmployeeMapper();
        var mappedResults= mapper.map(rawData);
        return mappedResults;
    }

Dependencies

  1. IRemoteApi
  2. FindEmployees
  3. EmployeeMapper

Now that we are dependent on an abstraction, we can provide an alternative implementation of that abstraction to our automated tests (a test double) that verify the behavior of GetEmployees. This is a good place to be. Our code is loosely coupled, easily testable, and does not easily break when implementation details in other objects change.

Refactoring: Replace DataModel with ViewModel

I have an application in which the UI is strongly bound to the data models.  I tried to make some modifications to the NHibernate mappings to better model the database and enable better querying. Since the UI was directly bound to the data models this meant that it have to follow navigation properties from one data model to another to get all the information it needed. Since the UI is no longer connected to the database, the app started generating data access exceptions all over the place. This is the reason that binding data models to the UI is not a good idea, and that data models should not properly be considered to be “domain” objects.

Consider the following simple WindowsForms application:

   1:      public partial class frmOwnerVisits : Form
   2:      {
   3:          public frmOwnerVisits()
   4:          {
   5:              InitializeComponent();
   6:  
   7:              this.Repository = new NHibernateRepository();
   8:          }
   9:  
  10:          private NHibernateRepository Repository { get; set; }
  11:  
  12:          private void btnLoadOwners_Click(object sender, EventArgs e)
  13:          {
  14:              // data is bound directly to the UI
  15:              var owners = this.Repository.All<Owner>();
  16:              this.cmbOwners.DataSource = owners;
  17:          }
  18:  
  19:          private void btnSavePets_Click(object sender, EventArgs e)
  20:          {
  21:              var owner = this.cmbOwners.SelectedItem as Owner;
  22:              foreach(var pet in owner.Pets)
  23:              {
  24:                  this.Repository.Save(pet);
  25:              }
  26:          }
  27:  
  28:          private void cmbOwners_SelectedIndexChanged(object sender, EventArgs e)
  29:          {
  30:              var owner = this.cmbOwners.SelectedItem as Owner;
  31:  
  32:              // Problem area: we're disconnected from the database at this point
  33:              // and we may not have pulled back all the visits for each pet.
  34:              var numberOfVisits = owner.Pets.SelectMany(p => p.Visits).Count();
  35:              this.lblNumberOfVisits.Text = numberOfVisits.ToString("D");
  36:          }
  37:  
  38:      }

The problem area for us at this point is in cmbOwners_SelectedIndexChanged. We want “Pet.Visits” on the data model because it enables easier querying, but having the properties there implies that they are navigable. We won’t know that we have a problem accessing the information at compile-time which results in harder-to-find and diagnose run-time exceptions.

   1:      private IEnumerable<Owner> GetOwnersWhoHaveNotBeenHereInAYear()
   2:      {
   3:          // Without fully expressed relationships, queries such as this would be much harder.
   4:          var results = this.Repository.All<Owner>()
   5:              .Where(row => row.Pets.Any(pet => pet.Visits
   6:                   .All(visit => visit.Date < DateTime.Now.AddYears(1))))
   7:              .ToList()
   8:              ;
   9:  
  10:          return results;
  11:      }

This is a simplified example. An ideal solution would be to create a complete ViewModel that represents the entire form. Let’s pretend that the example if much more complex and involves multiple cooperation user controls and tons of nasty event-driven mess. Let’s further pretend that we’ve decided that replacing Owner with OwnerViewModel would be a manageable chunk of work. How would we be sure we got everything that the UI depends on?

Steps

  1. Extract all methods that retrieve or persist data into an external helper class.
  2. Write Tests against those methods if you haven’t already.
  3. Create Empty ViewModels for each input and output to your Data Operations
  4. Create and test converter classes to translate between DataModels and ViewModels
  5. Replace inputs and outputs of your external helper class with ViewModels.
  6. If you’re working in a statically typed language, use the compiler errors to help you identify which properties and relationships are actually being used by the View.
  7. Fill in the properties and relationships used by the View on your ViewModels.
  8. Regression test the UI to try to find side-effects not covered by existing tests.
  9. Review and Refactor

 

1. Extract methods that retrieve or persist data into an external helper class.

If possible, write integration tests against the UI before making any changes at all.

The data access operations associated with Owner are “Get Owners”, “Save Pets”, and “Determine the number of visits for the owner.”

I extracted those methods into an OwnerController class that encapsulates the data operations.

   1:      public class OwnerController
   2:      {
   3:          public IEnumerable<Owner> GetOwners()
   4:          {
   5:              using (var repository = new NHibernateRepository())
   6:              {
   7:                  var owners = repository.All<Owner>().ToList();
   8:                  return owners;
   9:              }
  10:          }
  11:  
  12:          public int GetNumberOfVisits(Owner owner)
  13:          {
  14:              using (var repository = new NHibernateRepository())
  15:              {
  16:                  var numberOfVisits = repository
  17:                      .All<Visit>()
  18:                      .Count(visit => visit.Pet.Owner.OwnerId == owner.OwnerId)
  19:                      ;
  20:  
  21:                  return numberOfVisits;
  22:              }
  23:          }
  24:  
  25:          public void SavePets(IEnumerable<Pet> pets)
  26:          {
  27:              using (var repository = new NHibernateRepository())
  28:              {
  29:                  foreach (var pet in pets)
  30:                  {
  31:                      repository.SaveOrUpdate(pet);
  32:                  }
  33:              }
  34:          }
  35:  
  36:      }

Then I replaced the direct data access calls in my Form with calls into the OwnerController.

   1:      public partial class frmOwnerVisits : Form
   2:      {
   3:          public frmOwnerVisits()
   4:          {
   5:              InitializeComponent();
   6:  
   7:              this.Controller = new OwnerController();
   8:          }
   9:  
  10:          protected OwnerController Controller { get; set; }
  11:  
  12:          private void btnLoadOwners_Click(object sender, EventArgs e)
  13:          {
  14:              // data is bound directly to the UI
  15:              var owners = this.Controller.GetOwners();
  16:              this.cmbOwners.DataSource = owners;
  17:          }
  18:  
  19:          private void btnSavePets_Click(object sender, EventArgs e)
  20:          {
  21:              var owner = this.cmbOwners.SelectedItem as Owner;
  22:              this.Controller.SavePets(owner.Pets);
  23:          }
  24:  
  25:          private void cmbOwners_SelectedIndexChanged(object sender, EventArgs e)
  26:          {
  27:              var owner = this.cmbOwners.SelectedItem as Owner;
  28:  
  29:              // Problem area: we're disconnected from the database at this point
  30:              // and we may not have pulled back all the visits for each pet.
  31:              var numberOfVisits = this.Controller.GetNumberOfVisits(owner);
  32:              this.lblNumberOfVisits.Text = numberOfVisits.ToString("D");
  33:          }
  34:  
  35:      }

 

2. Write tests against the OwnerController if you haven’t already.

I suggest you always write your tests before creating the new class. As written above, the OwnerController doesn’t really allow for Unit Testing since NHibernate requires a connection to a database. NHibernate supports Sqlite for in-memory database testing and Entity Framework supports Sql Server CE. At some point I’d like to be able to stub in an In-Memory repository to the OwnerController so that I have no external dependencies for my tests, but that may be out of scope for the current operation.

3. Create Empty ViewModels for each input and output to your Data Operations

At this point, there are only 3 data models used by the OwnerController: Owner, Pet, and Visit. I have an idea about Visit so I’m not going to model it yet. I will create empty ViewModels for Owner and Pet though.

   1:      public class OwnerViewModel
   2:      {
   3:  
   4:      }
   5:  
   6:      public class PetViewModel
   7:      {
   8:  
   9:      }

4. Create and test converter classes to translate between DataModels and ViewModels

The OwnerController is going to be altered to return ViewModels instead of DataModels. However, the details of converting DataModels to ViewModels and back should really be dealt with separately. Whether you use a tool like AutoMapper or some custom mapper or converter interface for your transforms, you’ll need tests. These tests will be anemic at first since we don’t have any properties on our ViewModels yet. We’ll fill them in more as we go.

5. Replace inputs and outputs of your external helper class with ViewModels.

Depending on DataModels makes the UI brittle. This coupling needs to be completely broken. Nothing but parameter objects or view models should be exposed outside of your helper class. Data models should not leak into the UI for any reason.

In our case, the OwnerController will use the converters to create and expose viewmodels through it’s interface. The OwnerController now looks like this:

   1:      public class OwnerController
   2:      {
   3:          private readonly IBuilder _builder;
   4:  
   5:          public OwnerController(IBuilder builder)
   6:          {
   7:              _builder = builder;
   8:          }
   9:  
  10:          public IEnumerable<OwnerViewModel> GetOwners()
  11:          {
  12:              using (var repository = new NHibernateRepository())
  13:              {
  14:                  var owners = repository.All<Owner>().ToList();
  15:                  var viewModels = _builder
  16:                      .CreateEnumerable<OwnerViewModel>()
  17:                      .FromEnumerable(owners)
  18:                      ;
  19:                  return viewModels;
  20:              }
  21:          }
  22:  
  23:          public int GetNumberOfVisits(OwnerViewModel owner)
  24:          {
  25:              using (var repository = new NHibernateRepository())
  26:              {
  27:                  var numberOfVisits = repository
  28:                      .All<Visit>()
  29:                      .Count(visit => visit.Pet.Owner.OwnerId == owner.OwnerId)
  30:                      ;
  31:  
  32:                  return numberOfVisits;
  33:              }
  34:          }
  35:  
  36:          public void SavePets(IEnumerable<PetViewModel> petViewModels)
  37:          {
  38:              using (var repository = new NHibernateRepository())
  39:              {
  40:                  var petIds = petViewModels.Select(vm => vm.PetId).ToList();
  41:  
  42:                  var pets = repository.All<Pet>()
  43:                      .Where(pet => petIds.Contains(pet.PetId))
  44:                      .ToDictionary(pet => pet.PetId.Value)
  45:                      ;
  46:  
  47:                  var existingPets = petViewModels.Where(vm => vm.PetId.HasValue);
  48:                  var newPets = petViewModels.Where(vm => !vm.PetId.HasValue);
  49:  
  50:                  foreach(var petViewModel in newPets)
  51:                  {
  52:                      InsertPet(petViewModel, repository);
  53:                  }
  54:  
  55:                  foreach (var petViewModel in existingPets)
  56:                  {
  57:                      var pet = pets[petViewModel.PetId.Value];
  58:                      UpdatePet(pet, petViewModel, repository);
  59:                  }
  60:              }
  61:          }
  62:  
  63:          private static void UpdatePet(Pet pet, PetViewModel petViewModel, NHibernateRepository repository)
  64:          {
  65:              pet.Name = petViewModel.Name;
  66:              pet.Type = petViewModel.Type;
  67:              pet.Breed = petViewModel.Breed;
  68:              pet.BirthDate = petViewModel.BirthDate;
  69:  
  70:              repository.SaveOrUpdate(pet);
  71:          }
  72:  
  73:          private void InsertPet(PetViewModel petViewModel, NHibernateRepository repository)
  74:          {
  75:              var pet = _builder.Create<Pet>().From(petViewModel);
  76:              repository.SaveOrUpdate(pet);
  77:          }
  78:      }

Note that the implementation of line 23 drove us to add OwnerId to the OwnerViewModel.

The implementation of Update drove us to add Name, Type, Breed, and BirthDate to PetViewModel.

6. If you’re working in a statically typed language, use the compiler errors to help you identify which properties and relationships are actually being used by the View.

At this point, the OwnerController is starting to look okay. The OwnerVisits form doesn’t compile anymore mainly because it’s still using Owner, Pets, and Visits to display and edit its data. If we modify the form so that it interacts with OwnerViewModel and PetViewModel instead of Owner and Pet, we’ll get even more compiler errors. OwnerViewModel needs a reference to a Pets collection of type PetViewModel.

The compiler may not find everything. You’ll have to do a bit of manual regression to make sure you found everything. When you do find problems, be sure to document the problems in unit tests. This last is especially important in dynamic languages where you don’t get compiler hints.

7. Fill in the properties and relationships used by the View on your ViewModels.

The OwnerViewModel and PetViewModel now look like this:

   1:      public class OwnerViewModel
   2:      {
   3:          public int? OwnerId { get; set; }
   4:  
   5:          // snipped for brevity
   6:  
   7:          public IList<PetViewModel> Pets { get; set; }
   8:      }
   9: 
  10:      public class PetViewModel
  11:      {
  12:          public int? PetId { get; set; }
  13:  
  14:          public string Name { get; set; }
  15:  
  16:          public string Type { get; set; }
  17:  
  18:          public string Breed { get; set; }
  19:  
  20:          public DateTime? BirthDate { get; set; }
  21:      }

 

8. Regression test the UI to try to find side-effects not covered by existing tests.

In DataBinding scenarios—in both web and desktop applications—data binding is often done via magic strings. This can result in run-time errors not visible at compile time that are hard to test in an automated fashion. In manually regressing the UI behavior you should be able to detect run-time binding errors and add any properties necessary to make the UI work correctly again.

9. Review and Refactor

One thing I noticed while refactoring this code is that numberOfVisits is being calculated against the database every time the selected owner is changed in the combo box. This is inefficient. I’d like to add NumberOfVisits to the PetViewModel and write a function on the OwnerViewModel to aggregate visits per pet for display purposes. This will allow us to calculate the value in-memory and remove a function from the OwnerController. The calculation to determine the number of visits can be done as part of the conversion from DataModel to ViewModel.

Now the ViewModels look like this:

   1:      public class OwnerViewModel
   2:      {
   3:          public int? OwnerId { get; set; }
   4:  
   5:          // snipped for brevity
   6:  
   7:          public IList<PetViewModel> Pets { get; set; }
   8:  
   9:          public int GetNumberOfVisits()
  10:          {
  11:              if (Pets == null)
  12:                  return 0;
  13:  
  14:              var result =Pets.Sum(pet => pet.NumberOfVisits);
  15:              return result;
  16:          }
  17:      }
  18:  
  19:      public class PetViewModel
  20:      {
  21:          public int? PetId { get; set; }
  22:  
  23:          public string Name { get; set; }
  24:  
  25:          public string Type { get; set; }
  26:  
  27:          public string Breed { get; set; }
  28:  
  29:          public DateTime? BirthDate { get; set; }
  30:  
  31:          public int NumberOfVisits { get; set; }
  32:      }

The SelectedIndexChanged event is modified as follows:

   1:          private void cmbOwners_SelectedIndexChanged(object sender, EventArgs e)
   2:          {
   3:              var owner = this.cmbOwners.SelectedItem as OwnerViewModel;
   4:              var numberOfVisits = owner.GetNumberOfVisits();
   5:              this.lblNumberOfVisits.Text = numberOfVisits.ToString("D");
   6:          }

We now have a UI that is bound to ViewModels instead of DataModels. This decouples the UI from the database which prevents errors from occurring when attempting to access properties from related tables that haven’t been loaded. The UI can now change independently of the database. We can change the shape of the ViewModels without changing the shape of the underlying data, and vice versa. The steps to accomplish this decoupling were not particularly hard and have dramatically improved the reliability and flexibility of the code. We are one step closer to sitting our Form on top of a master ViewModel specifically designed for this UI.

Refactoring Q & A: When Should I Refactor?

Answer: When you have to spend too long figuring out what the code does.

Consider the following partial implementation of a ShoppingCart class.

public class ShoppingCart
{
    public ShoppingCart()
    {
        this._readOnlyDetails = new ReadOnlyCollection<OrderDetail>(_details);
    }

    private readonly List<OrderDetail> _details = new List<OrderDetail>();
    private readonly ReadOnlyCollection<OrderDetail> _readOnlyDetails;
    public ReadOnlyCollection<OrderDetail> Details
    {
        get
        {
            return _readOnlyDetails;
        }
    }

Our ShoppingCart class is simply hiding the internal storage mechanism of Details so that clients of ShoppingCart cannot add or remove products directly. For this reason, ShoppingCart must provide its own methods for modifying the OrderDetails collection. In this post I implemented this method and applied some heavy refactoring. Don’t read the implementation just yet.

Consider the requirements: When adding a product to the shopping cart, I must provide an OrderDetail that indicates the quantity of that product that has been ordered. Subsequent additions of the same product to the shopping cart should not result in additional order details. Rather, the existing OrderDetail should reflect the sum of all quantities of the product being ordered.

Imagine that you open the source file to see how this is done, but you have all the outlining collapsed so that all you see is method signatures. Imagine that these are the method signatures you see:

public void AddProduct(Product product, int quantity)
private OrderDetail FindOrCreateDetailForProduct(Product product)
private OrderDetail CreateDetailForProduct(Product product)
private OrderDetail FindDetailByProductSku(string productSku)
private bool HasDetailForProductSku(string productSku)

Just read each one of these method signatures. Each method states clearly exactly what it is going to do. Now for each of the method signatures, imagine that you were going to expand the outlining so you can see the method body. What code would you expect to find in each one. Seriously, pause for a moment and think about what you would expect to find in each method body. Got it? Okay, now read the actual implementations of these methods.

public void AddProduct(Product product, int quantity)
{
    var detail = FindOrCreateDetailForProduct(product);
    detail.Quantity += quantity;
}

private OrderDetail FindOrCreateDetailForProduct(Product product)
{
    var detail = HasDetailForProductSku(product.Sku) ?
        FindDetailByProductSku(product.Sku) :
        CreateDetailForProduct(product);
    return detail;
}

private OrderDetail CreateDetailForProduct(Product product)
{
    var detail = new OrderDetail() { Product = product };
    this._details.Add(detail);
    return detail;
}

private OrderDetail FindDetailByProductSku(string productSku)
{
    var detail = this.Details.Single(d => d.Product.Sku == productSku);
    return detail;
}

private bool HasDetailForProductSku(string productSku)
{
    return this.Details.Any(d => d.Product.Sku == productSku);
}

I’m sure that some of the implementations are different than you imagined, but are they different in any substantial way? Probably not. A method called HasDetailForProductSku can have only so many syntactic implementations, all of which are semantically identical. The method names and signatures state what our code is doing in English-language terms that are meaningful to a person. The bodies of these methods provide the syntax that is meaningful to the compiler. Class, method, parameter, and variable names are targeted at people. Their purpose is to express the intent of the code to people. Conditionals, loops, and other statements are the basic forms in which we code, but their target audience is the compiler. To put this in the form of a general principle: “Refactor when the semantic meaning of the code is hidden or obscured by the syntax required by the compiler,” or “Refactor when you have to spend too long figuring out what the code does.”

TDD Talking Point: TDD is more about API than Implementation

One of the points I tried to make in my talk about TDD yesterday is that TDD is more focused on the clarity and expressiveness of your code than on its actual implementation. I wanted to take a little time and expand on what I meant.

I used a Shopping Cart as an TDD sample. In the sample, the requirement is that as products are added to the shopping cart, the cart should contain a list or OrderDetails that are distinct by product sku. Here is the test I wrote for this case (this is commit #8 if you want to follow along):

[Test]
public void Details_AfterAddingSameProductTwice_ShouldDefragDetails()
{
    // Arrange: Declare any variables or set up any conditions
    //          required by your test.
    var cart = new Lib.ShoppingCart();
    var product = new Product() { Sku = "ABC", Description = "Test", Price = 1.99 };
    const int firstQuantity = 5;
    const int secondQuantity = 3;

    // Act:     Perform the activity under test.
    cart.AddToCart(product, firstQuantity);
    cart.AddToCart(product, secondQuantity);

    // Assert:  Verify that the activity under test had the
    //          expected results
    Assert.That(cart.Details.Count, Is.EqualTo(1));
    var detail = cart.Details.Single();
    var expectedQuantity = firstQuantity + secondQuantity;
    Assert.That(detail.Quantity, Is.EqualTo(expectedQuantity));
    Assert.That(detail.Product, Is.SameAs(product));
}

The naive implementation of AddToCart is currently as follows:

public void AddToCart(Product product, int quantity)
{
    this._details.Add(new OrderDetail()
                          {
                              Product = product, 
                              Quantity = quantity
                          });
}

 

This implementation of AddToCart fails the test case since it does not account for adding the same product sku twice. In order to get to the “Green” step, I made these changes:

public void AddToCart(Product product, int quantity)
{
    if (this.Details.Any(detail => detail.Product.Sku == product.Sku))
    {
        this.Details.First(detail => detail.Product.Sku == product.Sku).Quantity += quantity;           
    }
    else
    {
        this._details.Add(new OrderDetail()
        {
            Product = product,
            Quantity = quantity
        });                
    }

}

At this point, the test passes, but I think the above implementation is kind of ugly. Having the code in this kind of ugly state is still a value though because now I know I have solved the problem correctly. Let’s start by using Extract Condition on the conditional expression.

public void AddToCart(Product product, int quantity)
{
    var detail = this.Details.SingleOrDefault(d => d.Product.Sku == product.Sku);
    if (detail != null)
    {
        detail.Quantity += quantity;           
    }
    else
    {
        this._details.Add(new OrderDetail()
        {
            Product = product,
            Quantity = quantity
        });                
    }
}

The algorithm being used is becoming clearer.

  1. Determine if I have an OrderDetail matching the Product Sku.
  2. If I do, increment the quantity.
  3. If I do not, create a new OrderDetail matching the product sku and set it’s quantity.

It’s a pretty simple algorithm. Let’s do a little more refactoring. Let’s apply Extract Method to the lambda expression.

public void AddToCart(Product product, int quantity)
{
    var detail = GetProductDetail(product);
    if (detail != null)
    {
        detail.Quantity += quantity;           
    }
    else
    {
        this._details.Add(new OrderDetail()
        {
            Product = product,
            Quantity = quantity
        });                
    }
}

private OrderDetail GetProductDetail(Product product)
{
    return this.Details.SingleOrDefault(d => d.Product.Sku == product.Sku);
}

 

This reads still more clearly. This is also where I stopped in my talk. Note that it has not been necessary to make changes to the my test case because the changes I have made go to the private implementation of the class. I’d like to go a little further now and say that if I change the algorithm I can actually make this code even clearer. What if the algorithm was changed to:

  1. Find or Create an OrderDetail matching the product sku.
  2. Update the quantity.

In the first algorithm, I am taking different action with the quantity depending on whether or not the detail exists. In the new algorithm, I’m demoting the importance of whether the order detail already exists so that I can always take the same action with respect to the quantity. Here’s the naive implementation:

public void AddToCart(Product product, int quantity)
{
    OrderDetail detail;
    
    if (this.Details.Any(d => d.Product.Sku == product.Sku))
    {
        detail = this.Details.Single(d => d.Product.Sku == product.Sku); 
    }
    else
    {
        detail = new OrderDetail() { Product = product };
        this._details.Add(detail);
    }
    
    detail.Quantity += quantity;           
}

The naive implementation is a little clearer. Let’s apply some refactoring effort and see what happens.. Let’s apply Extract Method to the entire process of getting the order detail.

public void AddToCart(Product product, int quantity)
{
    var detail = GetDetail(product);

    detail.Quantity += quantity;
}

private OrderDetail GetDetail(Product product)
{
    OrderDetail detail;
    
    if (this.Details.Any(d => d.Product.Sku == product.Sku))
    {
        detail = this.Details.Single(d => d.Product.Sku == product.Sku); 
    }
    else
    {
        detail = new OrderDetail() { Product = product };
        this._details.Add(detail);
    }
    return detail;
}

This is starting to take shape. However, “GetDetail” does not really communicate that we may be creating a new detail instead of just returning an existing one. If we rename it to FindOrCreateOrderDetailForProduct, we may get that clarity.

public void AddToCart(Product product, int quantity)
{
    var detail = FindOrCreateDetailForProduct(product);

    detail.Quantity += quantity;
}

private OrderDetail FindOrCreateDetailForProduct(Product product)
{
    OrderDetail detail;
    
    if (this.Details.Any(d => d.Product.Sku == product.Sku))
    {
        detail = this.Details.Single(d => d.Product.Sku == product.Sku); 
    }
    else
    {
        detail = new OrderDetail() { Product = product };
        this._details.Add(detail);
    }
    return detail;
}

AddToCart() looks pretty good now. It’s easy to read, and each line communicates the intent of our code clearly. FindOrCreateDetailForProduct() on the other hand is less easy to read. I’m going to apply Extract Conditional to the if statement, and Extract Method to each side of the expression. Here is the result:

private OrderDetail FindOrCreateDetailForProduct(Product product)
{
    var detail = HasProductDetail(product) ? 
        FindDetailForProduct(product) : 
        CreateDetailForProduct(product);
    return detail;
}

private OrderDetail CreateDetailForProduct(Product product)
{
    var detail = new OrderDetail() { Product = product };
    this._details.Add(detail);
    return detail;
}

private OrderDetail FindDetailForProduct(Product product)
{
    var detail = this.Details.Single(d => d.Product.Sku == product.Sku);
    return detail;
}

private bool HasProductDetail(Product product)
{
    return this.Details.Any(d => d.Product.Sku == product.Sku);
}

Now I’ve noticed that HasProductDetail and FindDetailForProduct are only using the product sku. I’m going to change the signature of these methods to accept only the sku, and I’ll change the method names accordingly.

public void AddToCart(Product product, int quantity)
{
    var detail = FindOrCreateDetailForProduct(product);
    detail.Quantity += quantity;
}

private OrderDetail FindOrCreateDetailForProduct(Product product)
{
    var detail = HasDetailForProductSku(product.Sku) ? 
        FindDetailByProductSku(product.Sku) : 
        CreateDetailForProduct(product);
    return detail;
}

private OrderDetail CreateDetailForProduct(Product product)
{
    var detail = new OrderDetail() { Product = product };
    this._details.Add(detail);
    return detail;
}

private OrderDetail FindDetailByProductSku(string productSku)
{
    var detail = this.Details.Single(d => d.Product.Sku == productSku);
    return detail;
}

private bool HasDetailForProductSku(string productSku)
{
    return this.Details.Any(d => d.Product.Sku == productSku);
}

At this point, the AddToCart() method has gone through some pretty extensive refactoring. The basic algorithm has been changed, and the implementation of the new algorithm has been changed a lot. Now let me point something out: At no time during any of these changes did our test fail, and at no time during these changes did our test fail to express the intended behavior of the class. We made changes to every aspect of the implementation: We changed the order of the steps in the algorithm. We constantly added and renamed methods until we had very discrete well-named functions that stated explicitly what the code is doing. The unit test remained a valid expression of intended behavior despite all of these changes. This is what it means to say that a test is more about API than implementation. The unit-test should not depend on the implementation, nor does it necessarily imply a particular implementation.

Happy Coding!