The Aggressive Class vs The Laid Back Class

Oftentimes it’s very beneficial to be aggressive. You can be in sales, sports, real estate, marketing,  or Hollywood and an agressive attitude will most likely pay off. I think that as developers, having an agressive, “go-get-em” demeanor helps us keep on top of the game; we can learn new practices and technologies to help ourselves write better software. In contrast, developers who are laid-back, or even lazy, are going to have a much tougher road ahead as technology (inevitably) advances. It seems odd yet obvious to me that the code we write should be the complete opposite.  Take this ComputerBuilder class for example:

public class ComputerBuilder
{
	private readonly List<Part> monitorList = new List<Part>();
	private readonly List<Part> printerList = new List<Part>();

	public ComputerBuilder(string configFilename)
	{
		var configuration = ConstructConfiguration(configFilename);
		var database = new Database();

		var monitors = database.GetParts(PartType.Monitor);
		var monitorPrice = configuration.GetPrice(PartType.Monitor);
		foreach (var monitor in monitors)
		{
			if (monitor.IsInPriceRange(monitorPrice))
			{
				monitorList.Add(monitor);
			}
		}

		var printers = database.GetParts(PartType.Printer);
		var printerPrice = configuration.GetPrice(PartType.Printer);
		foreach (var printer in printers)
		{
			if (printer.IsInPriceRange(printerPrice))
			{
				monitorList.Add(printer);
			}
		}
	}

	private Configuration ConstructConfiguration(string filename)
	{
		// omitted code to build up a Configuration object from a file
	}

	public List<Part> GetMonitorsInPriceRange()
	{
		return monitorList;
	}

	public List<Part> GetPrintersInPriceRange()
	{
		return printerList;
	}
}

Look how “powerful” this class is. It accepts a conguration file’s filename in the constructor, reads some data from it and generates a couple lists of parts for use later. It also reads the file for a connection string to connect to adatabase. This class is very aggressive, it’s a real “go-getter”. There’s a few problems with this; I call this powerful, but that’s the wrong word; it’s messy and does too much. I breaks the Single Responsibility Principle in a most egregious manner. Time to refactor!

public class ComputerBuilder
{
	private Database database;
	private decimal monitorPrice;
	private decimal printerPrice;

	public ComputerBuilder(Configuration configuration, Database database)
	{
		this.database = database;
		monitorPrice = configuration.GetPrice(PartType.Monitor);
		printerPrice = configuration.GetPrice(PartType.Printer);
	}

	public List<Part> GetMonitorsInPriceRange()
	{
		var list = new List<Part>();
		var monitors = database.GetParts(PartType.Monitor);
		foreach (var monitor in monitors)
		{
			if (monitor.IsInPriceRange(monitorPrice))
			{
				list.Add(monitor);
			}
		}
		return list;
	}

	public List<Part> GetPrintersInPriceRange()
	{
		var list = new List<Part>();
		var printers = database.GetParts(PartType.Printer);
		foreach (var printer in printers)
		{
			if (printer.IsInPriceRange(printerPrice))
			{
				list.Add(printer);
			}
		}
		return list;
	}
}

So this is a lot better already. We didn’t have modify our methods, only the constructor. We don’t require our ComputerBuilder to know about the file system anymore. We also don’t need to open a database connection. It has become a little selfish and lazy, and that’s not necessarily a bad thing in this case. We can get better though. Let’s see what else we can do if we modify our methods a bit more. Not all the time you’ll be able to do sufficient refactoring to get your code to a point where it’s ideal, but the following is an ideal implementation of what our fake code is attempting to do.

public class ComputerBuilder
{
	private readonly IConfiguration configuration;
	private readonly IPartsRepository partsRepository;

	public ComputerBuilder(IConfiguration configuration, IPartsRepository partsRepository)
	{
		this.configuration = configuration;
		this.partsRepository = partsRepository;
	}

	IEnumerable<PART> GetPartsInPriceRange<PART>() where PART : IPart
	{
		var price = configuration.GetPrice<PART>();
		var parts = partsRepository.GetParts<PART>();
		return parts.Where(x => x.IsInPriceRange(price));
	}
}

The second refactoring is so laid back that it doesn’t even care what type of part you pass in to get the ones in the configured price range. All it worries about is that the type parameter, PART, fits the contract. Most everything has been abstracted into interfaces to allow for this chillaxed behavior. It’s not so aggressive anymore and if/when it throws a fit, you have as hard of a time figuring out why it’s mad.