Chapter 6: Improving Class Quality

In this chapter we will see how to refactor our code to be more cohesive and less coupled.

Refactoring methods with low-cohesion

Invoice could be refactored to make the constructor (and the class, for that matter) more cohesive by encapsulating customer information into a Customer class, encapsulating address information into an Address class and the Invoice class, and accepting a Customer parameter to the constructor that initializes a Customer property. The resulting refactoring would look like the following:

/// <summary>
/// Customer shape to encapsulate
/// name and address
/// </summary>
public class Customer
{
    public String FirstName { get; private set; }
    public String LastName { get; private set; }
    public Address Address { get; private set; }
    public Customer(string firstName, string lastName,Address address)
    {
        FirstName = firstName;
        LastName = lastName;
        Address = address;
    }
}
/// <summary>
/// Address shape to encapsulate
/// western-style addresses
/// </summary>
public class Address
{
    public string Street { get; private set; }
    public string City { get; private set; }
    public string Province { get; private set; }
    public string Country { get; private set; }
    public string PostalCode { get; private set; }
    public Address(string street, string city, string province,string country, string postalCode)
    {
        Street = street;
        City = city;
        Province = province;
        Country = country;
        PostalCode = postalCode;
    }
}
/// <summary>
/// Invoice class that makes use
/// of <seealso cref="Customer"/>
/// </summary>
public class Invoice
{
    public Customer Customer { get; private set; }
    public Invoice(IEnumerable<InvoiceLineItem>
        invoiceLineItems, Customer customer,
        Func<float, float, float> calculateGrandTotalCallback)
    {
        Customer = customer;
        InvoiceLineItems = new List<InvoiceLineItem>(invoiceLineItems);
        this.calculateGrandTotalCallback = calculateGrandTotalCallback;
    }
    //...
}

We now have an Invoice class that makes use of the Customer class, and the Customer class manages the customer (including address) responsibility.

This provides a much more cohesive constructor and encapsulates customer and address information so that code to initialize that type of information does not have to be repeated. We can use the code that used to be contained in Invoice anywhere else with Customer without having to repeat that code within the other classes that need it.

In other cases, a method with too many parameters is simply trying to do too many things.

/// <summary>
/// Invoice that uses callback for
/// grand total calculation
/// </summary>
public class Invoice
{
    private Func<float, float, float> calculateGrandTotalCallback;
    public Invoice(IEnumerable<InvoiceLineItem>
        invoiceLineItems,
        Func<float, float, float> calculateGrandTotalCallback)
    {
        InvoiceLineItems = new List<InvoiceLineItem>(invoiceLineItems);
        this.calculateGrandTotalCallback = calculateGrandTotalCallback;
    }
    public List<InvoiceLineItem> InvoiceLineItems { get; set; }
    public float TaxRate { get; set; }
    public bool CalculateTotals(out float invoiceSubTotal, out float invoiceTotalTax, out float invoiceGrandTotal)
    {
        invoiceSubTotal = 0;
        foreach (InvoiceLineItem invoiceLineItem in InvoiceLineItems)
        {
            invoiceSubTotal +=
              (float)((decimal)(invoiceLineItem.Price
              - invoiceLineItem.Discount)
              * (decimal)invoiceLineItem.Quantity);
        }
        invoiceTotalTax = (float)((Decimal)invoiceSubTotal * (Decimal)TaxRate);
        invoiceGrandTotal =
           calculateGrandTotalCallback(invoiceTotalTax,
           invoiceTotalTax);
        return true;
    }
    //...
}

CalculateTotals is a straightforward example that effectively returns three floating-point values from a method: calculating three values and assigning them to output parameters. What is common with methods with "multiple return values" is to return a Boolean result. This Boolean result is always true-and an indication that there's a design issue. In this example, the designer thought it would be useful to be able to query all the total values at the same time. The problem with this is that if only one value is needed, then the caller has to create a couple of dummy variables to needlessly store the unwanted values.

In order to refactor this, we simply need to perform Extract Method refactorings to split up the method into three methods: CalculateInvoiceGrandTotal,
CalculateInvoiceTotalTax, and CalculateLineItemSubtotal.

/// <summary>
/// Example of method with too many parameters
/// </summary>
public class Invoice
{
    private Func<float, float, float> calculateGrandTotalCallback;
    public Invoice(IEnumerable<InvoiceLineItem>
      invoiceLineItems,
      Func<float, float, float> calculateGrandTotalCallback)
    {
        InvoiceLineItems = new List<InvoiceLineItem>(invoiceLineItems);
        this.calculateGrandTotalCallback = calculateGrandTotalCallback;
    }
    public List<InvoiceLineItem> InvoiceLineItems { get; set; }
    public float TaxRate { get; set; }
    public bool CalculateTotals(out float invoiceSubTotal, out float invoiceTotalTax, out float invoiceGrandTotal)
    {
        invoiceSubTotal = 0;
        foreach (InvoiceLineItem invoiceLineItem in InvoiceLineItems)
        {
            invoiceSubTotal +=
              (float)((decimal)(invoiceLineItem.Price
              - invoiceLineItem.Discount)
              * (decimal)invoiceLineItem.Quantity);
        }
        invoiceTotalTax = (float)((Decimal)invoiceSubTotal * (Decimal)TaxRate);
        invoiceGrandTotal = calculateGrandTotalCallback(invoiceTotalTax, invoiceTotalTax);
        return true;
    }
    //...
}

We've now introduced three methods. CalculateInvoiceGrandTotalextracts the portion of code that calculates the invoice grand total and calculates the grand total based on an invoice subtotal and invoice total tax. CalculateInvoiceTotalTax extracts the portion of code that calculates the total tax based on an invoice subtotal. CalculateInvoiceSubTotal extracts the portion of code that sums the line item subtotals. Each of these methods takes on a single responsibility from the original CalculateTotals-which had taken on multiple responsibilities.

Another example is method, which is both a query and a modifier. Sometimes, this is easy to detect, as the word And is usually in the method name:
ChangeTaxAndCalculateGrandTotal. Sometimes, it's harder to detect and requires a bit of analysis of the method body. For example:

/// <summary>
/// Example Command AND Query method
/// </summary>
/// <param name="taxRate"></param>
/// <returns></returns>
public float CalculateGrandTotal(float taxRate)
{
    TaxRate = taxRate;
    float invoiceSubTotal = 0;
    foreach (InvoiceLineItem invoiceLineItem in InvoiceLineItems)
    {
        invoiceSubTotal +=
          (float)((decimal)(invoiceLineItem.Price
          - invoiceLineItem.Discount)
          * (decimal)invoiceLineItem.Quantity);
    }
    float invoiceTotalTax = (float)((Decimal)invoiceSubTotal * (Decimal)TaxRate);
    return calculateGrandTotalCallback(invoiceTotalTax, invoiceTotalTax);
}

This method first sets the TaxRate property, then proceeds to calculate the grand total based on the TaxRate property. In the case of this method, it's not clear from its signature (unlike ChangeTaxAndCalculateGrandTotal) that it is both a Command and a Query.

Command: A method that performs an action on an object; generally modifying state.
Query: A method or property that returns data to the caller.

In some circles, a Command is also known as a Modifier.

This can be refactored by performing the Separate Query from Modifier refactoring.

In our case, this is just a matter of removing the taxRate parameter from the method and removing the assignment to the TaxRate property, similar to the following code:

/// <summary>
/// Refactored to Query and not Command
/// </summary>
/// <returns></returns>
public float CalculateGrandTotal()
{
    float invoiceSubTotal = 0;
    foreach (InvoiceLineItem invoiceLineItem in InvoiceLineItems)
    {
        invoiceSubTotal +=
          (float)((decimal)(invoiceLineItem.Price
          - invoiceLineItem.Discount)
          * (decimal)invoiceLineItem.Quantity);
    }
    float invoiceTotalTax = (float)((Decimal)invoiceSubTotal * (Decimal)TaxRate);
    return calculateGrandTotalCallback(invoiceTotalTax, invoiceTotalTax);
}

Code that needs to calculate the grand total with a different tax rate should simply use the TaxRate property before calling CalculateGrandTotal. For example, instead of this:

float grandTotal = invoice.CalculateGrandTotal(.12f);

We'd refactor to this:

TaxRate = .12f;
float grandTotal = invoice.CalculateGrandTotal();

In a more complex Separate Query From Modifier refactoring, a new method or property would have to be created to separate the Query, and the method
performing the modification would remove the code that modifies state and is renamed (for example, ChangeTaxRateAndCalculateGrandTotal to CalculateGrandTotal) to be clear that it isn't modifying state. Calls to the method would have to be changed to the new query method, and a call to the modifier added before the call to the query.