I've inherited 200k lines of spaghetti code... what now?

We know spaghetti code is bad. We teach ourselves how to avoid writing it. But of course, there’s that gravitational force that pulls it from orbit and sends it falling into our laps…

Again, we have options. We can throw our hands in the air, tell our boss to have a nice life, and walk out the door. Or, we can start a campaign to rid the entire company of bad coding practices from the inside out, and wait 5 years to see any results.

Or, we can embrace it as an opportunity to show our team, our bosses, and our company that we can write good code. I like this option.

I use one refactoring pattern a lot to achieve the testability needed to make spaghetti code better - Extract Method.

Extract Method is a refactoring pattern from Martin Fowler’s Refactoring: Improving the Design of Existing Code. It’s so simple I probably don’t have to describe it to you. I will describe how important it is for dealing with that spaghetti code you’ve been “graced” with.

Let’s take an example from Contoso University from Microsoft’s Getting Started with Entity Framework 6 Code First using MVC 5. Here’s the InstructorController’s Index method when we open this project up for the first time:

// GET: Instructor
public ActionResult Index(int? id, int? courseID) {
    var viewModel = new InstructorIndexData();

    viewModel.Instructors = db.Instructors
        .Include(i => i.OfficeAssignment)
        .Include(i => i.Courses.Select(c => c.Department))
        .OrderBy(i => i.LastName);

    if (id != null) {
        ViewBag.InstructorID = id.Value;
        viewModel.Courses = viewModel.Instructors.Where(
            i => i.ID == id.Value).Single().Courses;
    }

    if (courseID != null) {
        ViewBag.CourseID = courseID.Value;
        // Lazy loading
        //viewModel.Enrollments = viewModel.Courses.Where(
        //    x => x.CourseID == courseID).Single().Enrollments;
        // Explicit loading
        var selectedCourse = viewModel.Courses.Where(x => x.CourseID == courseID).Single();
        db.Entry(selectedCourse).Collection(x => x.Enrollments).Load();
        foreach (Enrollment enrollment in selectedCourse.Enrollments)
        {
            db.Entry(enrollment).Reference(x => x.Student).Load();
        }

        viewModel.Enrollments = selectedCourse.Enrollments;
    }

    return View(viewModel);
}

Pretend we have to do something to this method, or something that touches this method. How would we write a test that preserves the behavior of this Controller’s Index action method, i.e. a “Characterization Test” (Michael Feathers)?

In this state, with all of this code in the Controller, it’s going to be hard to write a test first. We’d be forced to test whether or not the Index method returns the correct ActionResult and data, meaning there would be dependencies on HttpContext, meaning we’d have to work on some test doubles…

Let’s Extract Method to separate all this code from the Index method’s web-page-serving responsibilities and to isolate it so we can test it more easily. For more complex Extract Method refactorings, ReSharper can really help out - but cut/paste is just fine right now.

All this code is really doing is making a View Model to return to the UI. We can test this in isolation:

public InstructorIndexData GetInstructorIndexData(int? id, int? courseID) {

	//... same code as above

}

The Instructor Controller’s Index action method becomes this:

// GET: Instructor
public ActionResult Index(int? id, int? courseID) {
    var viewModel = GetInstructorIndexData(id, courseID);

    return View(viewModel);
}

Testing our new extracted method looks like this:

[Fact]
public void InstructorController_GetInstructorIndexData() {
    //Instructor: Kim Abercrombie Id: 9
    var controller = new InstructorController(new SchoolContext());
    var instructorIndexData = controller.GetInstructorIndexData(9, null);
    _output.WriteLine(instructorIndexData.Courses.ToJSON());
}

No mocking HttpContext, no weird acrobatics to break dependencies, just some separation of concerns so we can write a test.

It’s easy to get concerned about “what mocking framework should I use?” and “how do I mock this?”, but make sure you exhaust your ability to Extract Method when you’re faced with making spaghetti code testable. It might be the simplest thing you need to do.

For more interesting tidbits about testing, architecture, C# and more - make sure you sign up for my newsletter to get them delivered right to your inbox!

Related Posts: