Improve The Code – Answered

A Big Thank You from me to everyone who replied with answers to the Improve The Code programming challenge! The winner claims their coffee in anonymity.

I had initially planned to mention and critique the programming challenge answers people had sent in. I realised that this would make the blog post too long. So, to keep today’s post to a reasonable length, I am only showing my answer.

OK, let’s get into it.

The improvement I recommend are:

1. Prefer Construction to Initialisation

The original unit test uses object initialisation:

  var apple = new Product
  {
     Name = "Apple",
     UnitPrice = 0.35m
  };

Initialisation first calls the default constructor and then uses property setters to establish the object’s initial state. The problem with this approach is that it can lead to an inconsistent set up of the object state. For example, we can initialise apple without specifying the unit price:

  var apple = new Product
  {
     Name = "Apple"
  };

The unit price of this apple would be set to 0 by default. It would sell for free. Oops!

On the other hand, using construction, we can require that callers create Product instances with name and unitPrice: 

  var apple = new Product("Apple", 0.35m);

The Product class definition must include a constructor with parameters for product name and unit price:

  public class Product
  {
     public string Name { get; }
     public decimal UnitPrice { get; }

     public Product(string name, decimal unitPrice)
     {
        Name = name;
        UnitPrice = unitPrice;
     }
  }

Note: I have also removed the setter part of the properties. They were no longer required. Remove things you don’t need.

2. Encapsulate Behaviour

Please take a look at the initialisation of the two sales invoice lines:

  var line1 = new SalesInvoiceLine
  {
     Product = apple,
     Quantity = 4,
     Subtotal = 4 * apple.UnitPrice
  };
  var line2 = new SalesInvoiceLine
  {
     Product = banana,
     Quantity = 3,
     Subtotal = 3 * banana.UnitPrice
  };

In both instances, we repeated the Subtotal calculations. Isn’t it strange that the unit test does work which class SalesInvoiceLine should do? 

What is the point of having powerful tools of object-orientation if we don’t use them?

OK, let’s fix that and move the calculation of Subtotal inside the SalesInvoiceLine class:

  public class SalesInvoiceLine
  {
     public Product Product { get; }
     public int Quantity { get; 
     public decimal Subtotal => Product.UnitPrice * Quantity;

     public SalesInvoiceLine(Product product, int quantity)
     {
        Product = product;
        Quantity = quantity;
     }
  }

Now, every time we create a new invoice line, the Subtotal is automagically calculated for us:

var line = new SalesInvoiceLine(apple, 4);
var subtotal = line.Subtotal;   // 4 * $0.35 => $1.40

3. Initialise collections

In the original unit test, we externally set the collection of invoice lines onto the SalesInvoice:

  var invoice = new SalesInvoice
  {
     Lines = lines,
     LineCount = lines.Count,
     Total = lines.Sum(l => l.Subtotal)
  };

Note: LinesCount and Total are also not calculated inside SalesInvoice. They should be.

When we construct an object with references to internal collection types, we should initialise the collections. Neglecting this step, we run the risk of a call to add items into the uninitialised collection, raising a NullReferenceException—a serious bug.

Here is a better, more encapsulated definition of SalesInvoice:

  public class SalesInvoice
  {
     public decimal Total => Lines.Sum(x => x.Subtotal);
     public int LineCount => Lines.Count;
     private IList<SalesInvoiceLine> Lines { get; } = new List<SalesInvoiceLine>();

     public void Add(Product product, int quantity)
     {
        Lines.Add(new SalesInvoiceLine(product, quantity));
     }
  }

OK, we are done! I hope you found my recommendations useful.

Following are the complete code listings for the unit test and the classes.

The unit test shrinks down to this:

  using Xunit;
  using FluentAssertions;

  namespace OO
  {
     public class SalesInvoiceTests
     {
        [Fact]
        public void Add_Lines_To_SalesInvoice_And_Verify_They_Have_Been_Added()
        {
           var apple = new Product("Apple", 0.35m);
           var banana = new Product("Banana", 0.75m);

           var invoice = new SalesInvoice();
           invoice.Add(apple, 4);
           invoice.Add(banana, 3);

           invoice.Total.Should().Be(3.65m);
           invoice.LineCount.Should().Be(2);
        }
     }
  }

So much easier to read than before!

And here is the implementation code:

  using System.Collections.Generic;
  using System.Linq;

  namespace OO
  {
     public class SalesInvoice
     {
        public decimal Total => Lines.Sum(x => x.Subtotal);
        public int LineCount => Lines.Count;
        private IList<SalesInvoiceLine> Lines { get; } = new List<SalesInvoiceLine>();

        public void Add(Product product, int quantity)
        {
           Lines.Add(new SalesInvoiceLine(product, quantity));
        }
     }

     public class SalesInvoiceLine
     {
        public Product Product { get; }
        public int Quantity { get; }
        public decimal Subtotal => Product.UnitPrice * Quantity;

        public SalesInvoiceLine(Product product, int quantity)
        {
           Product = product;
           Quantity = quantity;
        }
     }

     public class Product
     {
        public string Name { get; }
        public decimal UnitPrice { get; }

        public Product(string name, decimal unitPrice)
        {
           Name = name;
           UnitPrice = unitPrice;
        }
     }
  }