Programmers: TDD your Legacy Code to Single Respnsibility Principle. Part 1 – Setup and Planning

TL;DR

This article series shows the importance of having unit tests. It also shows the importance of adhering to the Single Responsibility Principle (SRP), as we get a new requirement for our code base. We take a section of “legacy code” and refactor it to more closely follow SRP, all while maintaining the safety net of unit tests and leveraging Test Driven Development (TDD). We also cover using Dependency Injection in this TDD context.

This was originally going to be one post, but ended up WAY too long, so it will be a series of three posts:
Part 1 – Setup and Planning the Responsibility Transfer
Part 2 – The Glorious TDD Cycle
Part 3 – Dependency Injection For the New Class and Wrap Up

At various points in the article series, I link to .zip files on Github that correspond to commits at stages along our refactoring progression. To find the exact commit on the GitHub repo, just take the name of the .zip file and find the commit id on this page.

Introduction

In the previous article, we demonstrated how to cover an existing method, specifically the validateForm method of a Drupal 8 form implementation, with unit tests. In this article series, we’ll look at how we can leverage the fact that we have unit tests in place, and extract a class from the form implementation in order to more closely follow the Single Responsibility Principle (SRP). We’ll pretend that we get a new requirement for the app, and the logic we extract from the existing validateForm method has functionality that will be needed for this new requirement.

Why SRP?

SRP was coined by Robert C. Martin, and it is a very important principle in object oriented design. It primarily helps to maintain the “high cohesion, low coupling” that proper object oriented design strives for.  If you’ve ever had to make a change to an application, and had to do so in multiple sections of code that seemed to do the same (or very similar) thing, then you were probably working in a system that didn’t uphold SRP very well. This “duplication” of code sections that repeat the same (or very similar) functionality, also violates another term you’ve probably heard before: DRY (don’t repeat yourself).

Testability, maintainability, and readability often go hand-in-hand, and SRP helps us to gain all three by breaking dependencies, removing duplication, and improving code clarity.

A New Requirement

In the previous article, we had a Drupal 8 module with a single form (GuessForm class). In that form, the user would select a question, and then have to guess the right answer. The user would then submit the answer, and the form would validate the answer and indicate if it was right or not. The logic that actually determined whether or not the answer was right was in the validateForm method (FormInterface::validateForm) of the form implementation.

Let’s say that we have a new requirement. We want to allow the user to upload a CSV spreadsheet, which will contain guesses to multiple questions all in the same spreadsheet. We want to then present a results page that indicates whether the answers are correct for the corresponding questions.

For the new requirement, we would create a new form (let’s call it UploadGuessForm), which allows the user to upload the file, and then in the FormInterface::validateForm we would copy and paste the logic from the original GuessForm::validateForm method and tweak it for our specific case … right? I sure hope not!

We already have GuessForm::validateForm under test, so we know it works. The best course of action would be to extract the applicable logic of that method into it’s own class. That way, we don’t end up repeating any essence of that logic when implementing the new requirement.

In this series, we won’t look at the code that would be responsible for uploading the CSV, but rather we’ll look at refactoring the existing code into a separate class that the CSV-uploading code would be able to make use of.

Starting Where We Last Left Off

In the last post, the module was called form_validation_unit_test_module (here on GitHub). In this post, the name of the module will be form_validation_module_refactor_srp so that both repositories can exist independently.

It is important to run the existing unit tests after every change, to make sure the code is still in a functioning state. In starting this refactoring, for example, I checked in the original form_validation_unit_test_module module, and then edited the namespace references and files to update the module name to form_validation_module_refactor_srp (this commit). Right after doing that rename, I ran the unit tests again to make sure nothing broke.

For the aforementioned rename/refactor, and for the rest of the unit tests in this article, we’ll simply run all of the unit tests in the suite by executing this command (after switching to the “core” directory of your Drupal installation):

../vendor/bin/phpunit --group form_validation_module_refactor_srp

Planning Our Extraction / Original Responsibilities

Before doing any refactoring, lets look at the existing GuessForm:validateForm method and plan what functionality we’ll actually extract to the new class.

public function validateForm(array &$form, FormStateInterface $form_state)
{
  $values = $form_state->getValues();

  $selected_question_key = $values[GuessForm::select_list_key];
  $correct_answer_to_selected_question = $this->getAnswerToQuestion($selected_question_key);
  $entered_answer_value = $values[GuessForm::text_field_key];

  // 1
  if (empty($entered_answer_value))
    $form_state->setErrorByName(GuessForm::text_field_key, "You didn't guess anything!");

  // 2
  if ($selected_question_key == GuessForm::question_selection_default_key)
    $form_state->setErrorByName(GuessForm::select_list_key, "Please select a question to guess an answer!");

  // 3
  if ($entered_answer_value == GuessForm::text_field_default_value)
    $form_state->setErrorByName(GuessForm::text_field_key, "You should remove the default guess and enter your own.");

  if ($entered_answer_value != $correct_answer_to_selected_question)
  {
    $entered_answer_is_correct_for_a_question_other_than_what_was_selected = false;
    $question_selection_list = $this->getQuestionSelectionList();

    foreach ($question_selection_list as $question_selection_key => $question_selection_value)
    {
      $correct_answer_to_currently_iterated_question = $this->getAnswerToQuestion($question_selection_key);

      if ($correct_answer_to_currently_iterated_question === $entered_answer_value)
      {
        $entered_answer_is_correct_for_a_question_other_than_what_was_selected = true;
        break;
      }
    }

    if ($entered_answer_is_correct_for_a_question_other_than_what_was_selected)
    {
      // 4
      $form_state->setErrorByName(
        GuessForm::text_field_key, "You entered an incorrect answer to the question you were trying to guess, "
        . "but the answer was ironically correct for a different question. Change the question and then click 'guess again'!"
      );
    }
    else
    {
      // 5
      $form_state->setErrorByName(GuessForm::text_field_key, "You entered an incorrect answer to the question you were trying to guess.");
    }
  }
}

Some validation really is UI-centric, while other validation is specific to finding the correct answer to a question — this is a dichotomy that we want to make explicit by using different classes. The original method above has the following 5 responsibilities, corresponding to the labels in the comments:

  1. Make sure an answer was entered.
  2. Make sure a question was selected.
  3. Make sure the default answer was not submitted.
  4. Check if the answer was incorrect for the selected question, but might have been the correct answer to a different question.
  5. If responsibility 4 isn’t true, then validate the the given answer is not correct for ANY question.

Responsibilities 2 and 3 would more appropriately be left on the GuessForm class — selecting an answer, as we’ll as the providing of a “default question,” are specific to this UI form. However, it would benefit us to move the rest of the validations (1, 4, and 5) to a new class, as these are possible errors that can happen no matter whether we submit the question answers via form, CSV file upload (our new requirement), or even web service.

Extract Key Constants

Originally, we used constants on the GuessForm object to identify the various questions. Ultimately, we don’t want GuessForm to have to know about specific “questions,” so we’ll want to pull those constants out into a new class. We don’t want to just yet remove the from the GuessForm, in order that it’s unit tests will still pass, so we add this to our TODO list.

However, we can go ahead and create the new code that will hold the question keys. Enumerations don’t exist in PHP, so we create a “final” class to allow for identification of the questions:

final class QuestionAndAnswerKeys
{
  const favorite_number_key = 'favorite_number_key';
  const favorite_color_key = 'favorite_color_key';
  const favorite_aircraft_make_key = 'favorite_aircraft_make_key';
}

Part 1 Conclusion

In this part of the series, we described what the Single Responsibility Principle is, and why it’s important. We’ve laid out the responsibilities of the GuessForm::validateForm method that we want to delegate to our new class in order to fulfill the a new requirement that was given to us. Last, we created the QuestionAndAnswerKeys class to hold some constants, just to get that done before starting the TDD process in the next section.

Leave a Reply