Unit Testing VI - A Bad Constructor

An example of a bad (and virtually untestable) constructor


In the previous article in this series, we looked at running tests in the PhpStorm editor. In this one we'll add some minimal validation for our User class fields.


MODX logo

Validation

The User class fields need some validation. In this spectacularly bad example of coding practices, we'll add some validation in the User class constructor. It's a best practice to have a class constructor do as little work as possible. There are a number of reasons for this, but one is the difficulty of creating tests for actions taken in the constructor.

We're putting some validation in the constructor here for several reasons. One is that the user object is not really valid if any of its fields are not valid.

Some would argue that the constructor should simply set the field values, and the validation should go in another method, but users of our class would have to remember to call that method every time they created a new user. In addition, for some objects, the code might crash when the constructor received bad values for a particular field, creating a bug that might be very difficult to track down.

Another reason we're putting validation in the constructor is to show how to test methods called in the constructor, which you may have to do some day when trying to create tests for a class that you don't have the option to edit. We'll see how to do that in future articles, but first let's look at some validation code.

The code in this article is bad, bad, code. We won't even bother to put it up at GitHub or create tests for it, and you shouldn't add it to your project if you're actually performing the tests in these articles. Just look at the code, see if you can tell what's wrong with it, and read the comments after the code, then go to the next article.


Some Bad Code

Here's the code for our User class, which includes validation in the constructor:

<?php

    class User {
        protected $fields = array(
            'username' => '',
            'email' => '',
            'phone' => ''
        );

        protected $error = false;

    /* Note that there are two underscores before the word "construct" */
    function __construct($properties = array()) {
        if (!empty($properties)) {
            foreach ($properties as $fieldName => $value) {
                if (!array_key_exists($fieldName,
                    $this->fields)) {

                    /* invalid field */
                    $this->error = true;
                    return;
                } else {
                    switch ($fieldName) {
                        case 'username':
                            if (strlen($value) > 25) {
                                /* username too long */
                                $this->error = true;
                                return;
                            }
                            break;

                        case 'email':
                            if (strpos($value, '@') === false) {
                                /* invalid email */
                                $this->error = true;
                                return;
                            }
                            break;

                        case 'phone':
                            $pattern = '/^[0-9\-,.:]/';
                            if (! preg_match($pattern, $value)) {
                                /* invalid phone */
                                $this->error = true;
                                return;
                            }
                            break;
                    }
                    $this->set($fieldName, $value);
                }
            }
        }
    }

    public function getError() {
        return $this->error;
    }

    public function set($fieldName, $value) {
        $this->fields[$fieldName] = $value;
    }
    public function get($fieldName) {
        if (array_key_exists($fieldName, $this->fields)) {
            return $this->fields[$fieldName];
        } else {
            return '';
        }
    }

    public function save() {
        return true;
    }
}

What's Wrong with this Code?

It's a long list. First of all, the validation is extremely minimal. Most of the fields could be wildly invalid and still pass validation. Since this is just a bad example, I didn't bother to make decent validators (we'll see some decent ones in later articles).

More important, for our purposes, the class is virtually untestable. Since PHP constructors, when used properly, don't return a value, the class sets an error flag that people using our User class can check, but there's no indication of what caused the error.

Another problem is that the constructor aborts at the first error, which means that any code beyond that point won't execute and any further errors won't be detected. That would make our testing code very lengthy and inefficient.

To test this class at all, we'd have to feed the whole class multiple sets of field values and check the error flag dozens of times. Even then, it would be extremely difficult and cumbersome to test the individual validators. Moving the validators into separate methods (validateEmail() for example) would help, but there's still the problem of the error handling. We need a much better system for handling errors.

We'll see that in the next article.



Coming Up

In the next article in this series, we'll move our validation code and improve the error handling. We'll also see some stubs.



For more information on how to use MODX to create a website, see my website Bob's Guides, or better yet, buy my book: MODX: The Official Guide.

Looking for high-quality, MODX-friendly hosting? As of May 2016, Bob's Guides is hosted at A2 hosting. (More information in the box below.)



Comments (0)


Please login to comment.

  (Login)