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;
	}
}

The Aggressive Class

The Laid Back Class

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.

Related:

Post Footer automatically generated by Add Post Footer Plugin for wordpress.

About Chris Missal

Oh hey, I'm a Senior Consultant for Headspring in Austin, TX. I've been working in software professionally since 2006 and I really, really love it. I'm mostly in the Microsoft world, but enjoy building computer things of all sorts (to be vague). When I'm not slinging code, I'm probably out and about slinging discs, bowling balls, or good beer with great friends.
This entry was posted in Best Practices, Design Principles. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • http://blogs.lessthandot.com chrissie1

    No, no, a good programmer is by default lazy as a concequence he writes better and faster software so he can do less. And he learns new things to prevent himself from doing to much.

    The go-get-em programmer will do first and ask questions later. The lazy programmer thins firsts then chooses the path of least resistance. It was a lazy programmer that invented TDD and the SOLID principle because he was tired of solving the same bugs over and over again.

    That’s my excuse anyway ;-)..

  • Sean Stapleton

    Useful discussion, would be more useful if the code samples didn’t throw brain exceptions…

    Sample 1, line 34:
    return new Configuration(filename);

    Sample 3:
    You’ve lost a ton of angle brackets in the translation here.

    e.g., line 12 is something like:
    IEnumerable GetPartsInPriceRange () where PART : IPart

    … at least this is what i assume you were trying to convey.

  • Sean Stapleton

    So here is how i interpret sample 3:

    public interface IPart
    {
    bool IsInPriceRange(int price);
    }

    public interface IConfiguration
    {
    int GetPrice () where PART : IPart;
    }

    public interface IPartsRepository
    {
    List GetParts () where PART : IPart;
    }

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

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

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

  • http://www.lostechies.com/members/chrismissal/default.aspx Chris Missal

    Thanks for the heads up Sean! I think all the code typos are corrected now.

  • Michael A. Smith

    Excellent post Sean. I imagine you can further refactor it using the dynamic capabilities of C# 4.0:

    IEnumerable GetPartsInPriceRange()
    {
    dynamic price = configuration.GetPrice();
    dynamic parts = partsRepository.GetParts();
    return parts.Where(x => x.IsInPriceRange(price));
    }

    (assuming of course the IConfiguration, and IPartsRepository interfaces were defined as required).

    Did I say it would be “refactoring”? I meant it would be an egregious effort to make a more reusable but less maintainable component.

  • http://blog.trevorpower.com Trevor

    Nice post, good example, short and to the point.

  • http://maegancarberry.com/images/user/onlinepoker/

    Oh, its great!