Skip to content

Posts › Development

Development

Refactor data checking to cleaner implementation

Very often you find yourself in situations where you need to make your data pass some kind of checks or filters before doing something with them. This post shows how to make a first attempt approach better and cleaner

10 min read
Refactor data checking to cleaner implementation

Refactor data checking to cleaner implementation

Very often you find yourself in situations where you need to make your data pass some kind of checks or filters before doing something with them. This post shows how to make a first attempt approach better and cleaner. It will exhibits the following advantages:

  • It is cleaner
  • It is more succint and self-explanatory
  • Enforces Open-closed principle by isolating change
  • Enforces Single Responsibility by separating concerns
  • It has declarative style

The repository for this tutorial is available on github: https://github.com/robertogallea/data-checking-refactoring-example. The result of each refactoring round is represented by a separate branch.

Currently there are 6 branches:

The example

For explanation purposes, I describe a very simple example. However, the very same principles apply (and are even more useful) in more complex situations.

Suppose you have a NumberChecker class which performs some checks on a number. In particular it filters out:

  • Even numbers
  • Negative numbers
  • Numbers containing zeros
  • 4-digits number

In order to verify the class behavior, proper tests have been written using PHPUnit:

php
 1<?php
 2
 3
 4namespace tests;
 5
 6
 7use App\NumberChecker;
 8use PHPUnit\Framework\TestCase;
 9
10class NumberCheckerTest extends TestCase
11{
12    /**
13     * @test
14     * @dataProvider checkFalse
15     */
16    public function it_returns_false($number)
17    {
18        $checks = new NumberChecker();
19
20        $this->assertFalse($checks->execute($number));
21    }
22
23    public function checkFalse()
24    {
25        return [
26            [2],
27            [101],
28            [-5],
29            [1223],
30        ];
31    }
32
33    /**
34     * @test
35     * @dataProvider checkTrue
36     */
37    public function it_returns_true($number)
38    {
39        $checks = new NumberChecker();
40
41        $this->assertTrue($checks->execute($number));
42    }
43
44    public function checkTrue()
45    {
46        return [
47            [3],
48            [111],
49            [99],
50            [999],
51        ];
52    }
53}

There are two tests for checking invalid and valid numbers respectively. They use data providers for testing multiple values at once.

First approach

The simplest and rawest approach is to implement the execute() method of NumberChecker class using a battery of if-then-else statements applying the required rules one after each other.

php
 1<?php
 2
 3
 4namespace App;
 5
 6
 7class NumberChecker
 8{
 9    public function execute($number)
10    {
11        if ($number % 2 === 0) { // even
12            return false;
13        } elseif (strpos((string)$number, '0')) { // contains zeros
14            return false;
15        } elseif ($number < 0) { // negative
16            return false;
17        } elseif ($number > 999 && $number < 9999) { // four digits
18            return false;
19        }
20
21        return true;
22    }
23}

This is perfectly working, and in fact the tests all pass. However, even in this simple case, consider the following problems:

  • Someone who reads the code incurs in cognitive load when trying to understand what it does (comments should not be required!);
  • If another rule has to be added, the code must be changed.
  • If many rules are required, the method will grow soon.

Refactor - Round 1: extract methods

The first step involves refactoring that would reduce the cognitive load of the code. It is simple and just consists in extracting the if conditions inside private class members:

php
 1?php
 2
 3
 4namespace App;
 5
 6
 7class NumberChecker
 8{ 
 9    public function execute($number)
10    {
11        if ($this->isDivisibleBy2($number)) {
12            return false;
13        } elseif ($this->containsZeros($number)) {
14            return false;
15        } elseif ($this->isNegative($number)) {
16            return false;
17        } elseif ($this->hasFourDigits($number)) {
18            return false;
19        }
20
21        return true;
22    }
23
24    private function isDivisibleBy2($number)
25    {
26        return $number % 2 === 0;
27    }
28
29    private function containsZeros($number)
30    {
31        return strpos((string)$number, '0');
32    }
33
34    private function isNegative($number)
35    {
36        return $number < 0;
37    }
38
39    private function hasFourDigits($number)
40    {
41        return $number > 999 && $number < 9999;
42    }
43}

Nice start, this refactoring produces more readable code, in fact comments are no longer required, but makes it more verbose. This leads us directly to round 2.

Refactor - Round 2: extract classes

In order to reduce the amount of code in NumberChecker class, checking methods could be easily delegated to specific classes, whose responsibility is just to define and perform a single check.

For this purposes, firstly define a common Check interface, that concrete checks will implement:

php
 1<?php
 2
 3
 4namespace App;
 5
 6
 7interface Check
 8{
 9    public function check($number): bool;
10}

Then, four classes are defined, one for each required check:

  • IsDivisibleBy2
  • ContainsZeros
  • IsNegative
  • HasFourDigits

IsDivisibleBy2.php

php
 1<?php
 2
 3
 4namespace App;
 5
 6
 7class IsDivisibleBy2 implements Check
 8{
 9    public function check($number): bool
10    {
11        return $number % 2 === 0;
12    }
13}

ContainsZeros.php

php
 1<?php
 2
 3
 4namespace App;
 5
 6
 7class ContainsZeros implements Check
 8{
 9    public function check($number): bool
10    {
11        return strpos((string)$number, '0');
12    }
13}

IsNegative.php

php
 1<?php
 2
 3
 4namespace App;
 5
 6
 7class IsNegative implements Check
 8{
 9    public function check($number): bool
10    {
11        return $number<0;
12    }
13}

HasFourDigits.php

php
 1<?php
 2
 3
 4namespace App;
 5
 6
 7class HasFourDigits implements Check
 8{
 9    public function check($number): bool
10    {
11        return $number > 999 && $number < 9999;
12    }
13}

The NumberChecker class is changed by instantiating and evaluating the required check inside each if statement, private methods could be removed now:

php
 1?php
 2
 3
 4namespace App;
 5
 6
 7class NumberChecker
 8{
 9    public function execute($number)
10    {
11        if ((new IsDivisibleBy2())->check($number)) {
12            return false;
13        } elseif ((new ContainsZeros())->check($number)) {
14            return false;
15        } elseif ((new IsNegative())->check($number)) {
16            return false;
17        } elseif ((new HasFourDigits())->check($number)) {
18            return false;
19        }
20
21        return true;
22    }
23}

We are getting closer. Now the code is more concise. Also, it enforces SRP (Single Responsibility Principle), since NumberChecker function is responsible just for applying the checks, not for defining it.

By now it has just one remaining smell, too much if-then-elses. This brings us straight to the next round of the refactoring.

Refactor - Round 3: move checks to array

Too much if-then-elses are ugly, and are kind-of code duplication. This last step is aimed to remove them and make code more declarative-fashioned.

To achieve such purpose, concrete checks classes are moved in an array class property, and checks are performed one by one inside a foreach loop:

php
 1<?php
 2
 3
 4namespace App;
 5
 6
 7class NumberChecker
 8{
 9    private array $checks = [
10        IsDivisibleBy2::class,
11        ContainsZeros::class,
12        IsNegative::class,
13        HasFourDigits::class
14    ];
15
16    public function execute($number)
17    {
18        foreach ($this->checks as $check) {
19            if ((new $check)->check($number)) {
20                return false;
21            }
22        }
23
24        return true;
25    }
26}

That sounds very readable and isolates the checks definition from the actual code which performs the checks. Moreover, you could store the $checks array inside a configuration and define which checks you actually want dynamically at runtime.

Refactoring - round 4: NumberChecker is a Check too

This is already a sound implementation, but it could be even better. It could be further improved indeed.
Looking at the code, the NumberChecker class can be considered a wrapper to a composite check itself, so we can (and should) extract a CompositeCheck class implementing the very same Check interface.

As first step, let's conform the body of NumberChecker class to the published Check interface, by exracting a check() method, which is wrapped into the existing execute() method.

php
 1<?php
 2
 3
 4namespace App;
 5
 6
 7class NumberChecker
 8{
 9    private array $checks = [
10        IsDivisibleBy2::class,
11        ContainsZeros::class,
12        IsNegative::class,
13        HasFourDigits::class
14    ];
15
16    public function execute($number): bool
17    {
18        return $this->check($number);
19    }
20
21    public function check($number): bool
22    {
23        foreach ($this->checks as $check) {
24            if ((new $check)->check($number)) {
25                return false;
26            }
27        }
28
29        return true;
30    }
31}

Note that is required not to introduce breaking changes in the existing NumberChecker public interface, otherwise we could have simply renamed execute() to check().

Then we can extract the CompositeChecker class by moving check() method and $checks array to a new class implementing Check interface:

CompositeChecker.php

php
 1<?php
 2
 3
 4namespace App;
 5
 6
 7class CompositeChecker implements Check
 8{
 9    private array $checks = [
10        IsDivisibleBy2::class,
11        ContainsZeros::class,
12        IsNegative::class,
13        HasFourDigits::class
14    ];
15
16    public function check($number): bool
17    {
18        foreach ($this->checks as $check) {
19            if ((new $check)->check($number)) {
20                return false;
21            }
22        }
23
24        return true;
25    }
26}

while NumberChecker becomes a simple wrapper for it:

NumberChecker.php

php
 1<?php
 2
 3
 4namespace App;
 5
 6
 7class NumberChecker
 8{
 9    public function execute($number): bool
10    {
11        return (new CompositeChecker())->check($number);
12    }
13}

This could seem rather useless refactoring. However it opens the way to interesting opportunities... Let's go the next step.

Refactor - Round 5: Generalize to generic composed checks

The last change opens to a brand new class of composite filters, defined by stacking a set of simpler checks. Under this new perspective, the array of checks should not be hardcoded inside the class, but it should be excerpted from the constructor. This brings a dual advantage:

  • Checks are not static but can be configured during instantiation
  • Mock check implementations could be used in the tests, making the class more testable.

CompositeCheck is now changed as follows:

CompositeCheck.php

php
 1<?php
 2
 3
 4namespace App;
 5
 6
 7class CompositeChecker implements Check
 8{
 9    private array $checks;
10
11    public function __construct(array $checks)
12    {
13        $this->checks = $checks;
14    }
15
16    public function check($number): bool
17    {
18        foreach ($this->checks as $check) {
19            if ((new $check)->check($number)) {
20                return false;
21            }
22        }
23
24        return true;
25    }
26}

While our four checks are moved back into NumberCheckers, and passed to CompositeCheck construcor. This is reasonable since it is NumberChecker responsibility to set the type of checks that should be actually performed.

NumberChecker.php

php
 1<?php
 2
 3
 4namespace App;
 5
 6
 7class NumberChecker
 8{
 9    public function execute($number): bool
10    {
11        $checks = [
12            IsDivisibleBy2::class,
13            ContainsZeros::class,
14            IsNegative::class,
15            HasFourDigits::class
16        ];
17
18        return (new CompositeChecker($checks))->check($number);
19    }
20}

That's all, at least for now. Simple, elegant, testable, and respectful of SOLID principles.

Conclusion

A simple yet useful refactoring is applied to a data checking routine. It separates the checking logic from code definition, enforcing SR and OCP principles of SOLID. Also it results more readable and follows a declarative approach.

If you have questions, suggestions, critics, please use the comment section below!