Applied Abstraction (Part 2)

Part 2 – Looking at the code…

The Code

The code we will review has come from a real system for the vehicle hiring trade. The company hires out vehicles to the normal public, and this system deals with tracking everything related to the vehicle hire. The code we are going to see is pretty simple, which is good because it allows us to focus on the design issues without getting bogged down. Take a look at listing 1 below.

public partial class assignmentForm : Form {
    public assignmentForm() {
        // ...
    }
    private void assignButton_Click(object sender, EventArgs e) {
        Customer customer = FindCustomer();
        AssignCustomer(customer);
    }
    private void unassignButton_Click(object sender, EventArgs e) {
        Customer customer = FindCustomer();
        UnAssignCustomer(customer);
    }
    private void AssignCustomer(Customer customer) {
        _currentVehicle.Assignee = customer.ID;
        _currentVehicle.AssigneeName = customer.Name;
    }
    private void UnAssignCustomer(Customer customer) {
        _currentVehicle.Assignee = -1;
        _currentVehicle.AssigneeName = String.Empty;
    }
    private Customer FindCustomer() {
        // ...
    }
    public void Save() {
        // ...
    }
    private Vehicle _currentVehicle;
}

Listing 1

The application is in WinForms, and we are looking at the AssignmentForm. This form allows the user to set up a new vehicle hiring by assigning a customer to a vehicle, and it also deals with a vehicle return which involves unassigning the customer from the vehicle. The save method deals with persistence but has been omitted. The form has two buttons, AssignButton and UnAssignButton, which deal with instigating the assigning and unassigning logic respectively.

Figure 1 - Dependencies of the AssignmentForm class

Figure 1 shows the design so far. The AssignmentForm is associated with Vehicle because it contains an internal field, and it has a dependency on the Customer class too. You might be thinking that there’s nothing very interesting about this diagram, but there is. There are two business methods on the AssignmentForm class, and there is no association between Vehicle and Customer classes, both of which are cause for concern.

What’s wrong with the code?

Our goal is to identify the design flaws in the code and uncover the nature of the design decisions and what they really mean to the software. In some cases it is enough just to know that a design is wrong without needing to understand the underlying theory of why it’s wrong, nor the need to appreciate its consequences on the software. Without the knowledge of the reasons why a design choice might be a concern, the more subtle design blemishes will go undetected with the potential to spoil the hard work of the designer when the design buckles as new requirements come.

Here are some observations of the code so far:
1. AssignCustomer and UnAssignCustomer are dependent on business policy details
2. The methods contain duplication
3. Method names encode implementation details
4. Weak object orientation

AssignCustomer and UnAssignCustomer are dependent on business policy details

AssignCustomer takes a Customer instance and, using the current Vehicle object (_currentVehicle), performs the assignment of the Customer to the Vehicle by taking the properties from the Customer object and manually assigning them onto properties of the Vehicle object. UnAssignCustomer is very similar, only in this case a –1 replaces the Customer ID from the AssignCustomer routine, presumably to indicate no assignment and String.Empty is assigned to the AssigneeName property of the Vehicle for the same reason.

A number of issues exist with this code, but here it’s the policy details I want to cover here. When we talk about policy in software, we are talking about the what, not the how. In listing B earlier, the policy for retrieving Result objects based on analyteName was hidden behind the GetResultsFor method on AnalysisRun. The Analyser wants a list of results, but doesn’t care about how the retrieval policy is implemented – this is what we strive for, continually hiding implementation details away from public view. In both of these two assignment methods the end-user, the AssignmentForm, has been exposed to details in an unhealthy manner. The form class not only performs its role as a user interface, but it also deals with implementing the business rules for tracking a vehicle and its assigned customer. If the business rules change at any time, the user interface will be forced to change for the wrong reasons. These two routines should only be dependent on what must happen to track a vehicle and its customer, not how this is implemented in the code.

NB: This section below could be a round-up piece for the whole section 1.

With the implementation details sitting in the user interface it is clear there is misplaced behaviour. The policy details belong not on a user interface class but on a domain class or something similar. The domain model has the role of representing the business domain, its relationships and its policies. Centralising business logic in the domain model keeps all of the important details in one place, and through abstraction these details are exposed to the rest of the software in a way that keeps the how hidden behind the what.

The methods contain duplication

We have two key business events, hiring and returning a vehicle, and both are implemented in the user interface. The implementation details of how the software links a Customer and Vehicle is exposed in both of the assignment methods. It is important to realise that between the two assignments methods there are two individual business policies being implemented. The first implements the vehicle hire policy; the second handles the vehicle return policy. It is the design choices for how to implement these policies that has been duplicated, as for both cases the properties on the vehicle are modified to reflect a different state, and this is repeated in each of the methods.

Method names encode implementation details

Both assignment methods encode within their names the actions that take place when they are invoked. When a customer hires a vehicle he is assigned to it that we know for sure. Similarly when the customer returns the vehicle he is unassigned from it. While this might not seem like much of an issue, what it highlights is the affects of depending on implementation details. The real phenomena to be captured here is the actual business event, not the actions that result from it. A customer hires a vehicle, and later he returns it. The user interface should be focused on capturing business events and firing them onto the lower layers for processing. As they are the method names badly represent what is taking place in the real-world that this software is supporting. Again it is worth stating that the names are focused on details rather than the abstract business events they are handling.

Weak object orientation

As there are a number of different design flaws in the code, it has the feel of being constructed from procedural ideals. The AssignmentForm class acts as a controller, controlling what happens to realize the business events. A better approach is to coordinate and delegate the controlling behaviour across the objects, which in itself improves separation of concerns and it allows the objects themselves to be more responsible software citizens. Take a look at the two domain classes in listing 2.

public class Customer {
        public int ID {
            get {
                return _id;
            }

            set {
                _id = value;
            }
        }
        public string Name {
            get {
                return _name;
            }

            set {
                _name = value;
            }
        }
        private int _id;
        private string _name;
}

public class Vehicle {
	public int Assignee {
       		get {
             		return _assignee;
		}
            	set {
             		_assignee = value;
            	}
	}
	public string AssigneeName {
       		get {
             		return _assigneeName;
            	}
            	set {
             		_assigneeName = value;
		}
	}
	private string _assigneeName;
	private int _assignee;
}

Listing 2
The two domain classes contain no real useful behaviour, adorned with nothing other than getters and setters. Given we know there is behaviour elsewhere in the system related to these classes, this indicates a weak realization of object oriented principles. A reason to localise the behaviour and state together as a class is because they often change together, and keeping them in the same place provides a good level of protection against the changes rippling out across the system. Using information hiding and encapsulation, a client of a class is forced to request a state change by passing a message across its public interface. If no such protocol exists on the class, then it must exist elsewhere, and this results in misplaced responsibilities and a breakdown in the separation of concerns. When you spot a class that contains nothing but getters and setters, it can be a sign that you are working with an anaemic domain model, see Fowler, http://www.martinfowler.com/bliki/AnemicDomainModel.html.

A final comment to make at this point is that there is no expression of the true relationships of the domain within the software. This is a shame because creating a rich representation of the domain is what brings a lot of value to the object oriented approach. We’ll come back to this point later.

Advertisements
  1. No trackbacks yet.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: