Variable cleaning class

Question

I've created a class that's used for cleaning variables for use. You set a name, value and options and it will set the value if it matches the criteria/options.

It is used like this:

$var = new Variable();
$var->clean('username', $_POST['username'], '3,13,alphanumeric,True');
echo 'Username', $var->clean("username");
echo 'Username', $var->html("username");

It seems very messy and as though it could be done in a better way.

<?php
include_once __DIR__ . '/header.php';

class Variable
{
    var $clean = array(); // Filtered data
    var $html = array(); // Escaped htmlentities() data

    /**
     * variable->addClean()
     *
     * @param mixed $name - Name of clean value
     * @param mixed $value - Value to clean
     * @param string $options - Options in the format specified
     * @return mixed - Clean value.
     *
     * Options format:
     *    {min length} - set to -1 to ignore.
     *    {max length} - set to -1 to ignore.
     *    {type} - set to null to ignore. Types:
     *       - Numeric.
     *       - AlphaNumeric.
     *       - Alpha.
     *    {trim} - default is true.
     *
     *    eg. 
     *    $var = new Variable();
     *    $var->clean('username', $_POST['username'], '3,13,alphanumeric,True');
     *    echo 'Username', $var->clean("username");
     *
     */
    public function clean($name, $value = null, $options = '')
    {
        if ($value === null && $name !== null)
            return isset($this->clean[$name]) ? $this->clean[$name] : null;

        else if ($value !== null && $name !== null) {
            if (!empty($options)) {
                list($minlen, $maxlen, $type, $trim) = array_pad(explode(',', (string)$options), 4, '');

                if (strtolower($trim) !== 'false')
                    $value = trim($value);

                if ($minlen !== '-1')
                    if (!(strlen($value) > $minlen))
                        return false;

                if ($maxlen !== '-1')
                    if (!(strlen($value) < $maxlen))
                        return false;

                switch (strtolower($type)) {
                    case 'numeric':
                        if (is_numeric($value))
                            $this->clean[$name] = intval($value);
                        break;
                    case 'alphanumeric':
                        if (ctype_alnum($value))
                            $this->clean[$name] = $value;
                        break;
                    case 'alpha':
                        if (ctype_alpha($value))
                            $this->clean[$name] = $value;
                        break;
                    default:
                        $this->clean[$name] = $value;
                        break;
                }
            } else
                $this->clean[$name] = $value;
        }
        return isset($this->clean[$name]);
    }

    public function isClean($name)
    {
        return isset($this->clean[$name]);
    }

    public function html($name)
    {
        if (!isset($this->html[$name]) && isset($this->clean[$name]))
            $this->html[$name] = htmlentities($this->clean[$name], ENT_QUOTES, 'UTF-8');

        return (isset($this->html[$name]) ? $this->html[$name] : null);
    }
}

Answer 1

Yes, your class does too much.

First of all, it actually lying: It does not handle one variable as the name of the class suggest, but it does handle multiple variables at once.

Second, I do not like the overloading of the clean method. It is used both for getting a value inside, and for getting the same value back outside. Why are there not two methods?

Third point: Your clean method does too much! It should not be its responsibility to actually decide which filter to apply - this should be the task of a dedicated class that just deals with on single aspect.

Summarizing all this, your list of improvements would be like this:

a) Get rid of the multi-value-behaviour. One class instance per value. If you really need multiple values inside a structure, use a different class.

b) Move the actual filtering out of the class, into dedicated filter elements that do one task very good, but nothing else.

For example, create an "Alphanumeric" class that is configured with the rest of the parameters "3,13,true" that does exactly deal with strings, checks their min and max length and optionally trims them.

Create another class that deals with numeric representation. I doubt it would be useful to check for a string length in this case, but your implementation dictates that it has to be like this. On the other hand, there is no way to implement a min or max value.

The checking class should be passed as a parameter to the constructor of your class Variable, which means that you have to create the checking class beforehand.

c) Passing a string as configuration would then be somehow obsolete, because your new checking classes do not have any other parameters, so giving them four separate parameters isn't likely to overflow the parameter list. But remember that you do not need to make every class' parameter list the same - if some class does not need a configuration value, you can omit it. If another class needs more: Add one.

d) In the end you might come to a point where creating more than one instance of Variable is cumbersome because of all the additional stuff. This is where a factory comes in handy. It would take a configuration array that defines which instances of variable cleaners are needed, instantiates all necessary classes and would probably return an array of Variable objects.

e) One final suggestion: When implementing different classes for basically the same task, use an interface as a type hint to guarantee that you never mix something up. When using an IDE you get code completion as a bonus. :)