Alternative for too many switch case for cyclomatic complexity reduction?

Question

I have a function for checking the type of a $value is valid or not. My current code is a simple switch case with too many cases exceeding the cyclomatic complexity to 17. I need to add more cases as well as reduce the complexity.

 /**
 * Check type of attribute value
 * @param $type
 * @param $value
 * @return bool
 */
public function typeCheck($type, $value)
{
    $this->value = $value;
    switch ($type) {
        case 'string':
            return is_string($value) || is_integer($value) || is_bool($value);
        case 'StringNotNull':
            return is_string($value);
        case 'LongStringNotNull':
            return is_string($value);
        case 'SuperLongStringNotNull':
            return is_string($value);
        case 'FortyStringNotNull':
            return is_string($value) && (strlen($value) < 41);
        case 'integer':
            return is_numeric($value);
        case 'positiveInteger':
            return is_numeric($value) && ($value > 0);
        case 'boolean':
            return is_bool($value) || ($value == 'true' || $value = 'false');
        case 'float':
            return is_numeric($value);
        case 'decimal':
            return is_numeric($value);
        case 'PositiveDimension':
            return is_numeric($value) && ($value > 0);
        case 'Dimension':
            return is_numeric($value) && (strlen($value) < 13);
        case 'Barcode':
            $validator = $this->getBarcodeValidator();
            $validator->setBarcode($value);
            return is_string($value) && (strlen($value) < 17 && $validator->isValid());
        case 'dateTime':
            return true;
        case 'normalizedString':
            $this->value = strip_tags($value);
            return is_string($value);
        default:
            return is_string($value);
    }
}

Any better way around?

score 2 · Accepted Answer · edited Sep 01 '20 at 12:26

2

You could replace the switch with a data structure:

public function typeCheck($type, $value) {

    $typeTestMap = [
        'string' => function($value) { return is_string($value) || is_integer($value) || is_bool($value); },
        'FortyStringNotNull' => function($value) { return is_string($value) && (strlen($value) < 41); },
        ...
    ];

    if (isset($typeTestMap[$type])) {
        return $typeTestMap[$type]($value);
    } else {
        return is_string($value);
    }
}

edited Sep 01 '20 at 12:26

Milind Singh

296
6
23

answered Aug 31 '18 at 09:06

Barmar

741,623
53
500
612

Thanks a lot. I was not aware of PHP supporting anonymous function. Also, my PhpStorm still throws a syntax error while writing the code, do you already faced this issue? – Milind Singh Aug 31 '18 at 10:02
1

You can't use an array as the initial value of a static variable. I've recoded it to just use a normal local variable. – Barmar Aug 31 '18 at 10:08

score 1 · Answer 2 · answered Aug 31 '18 at 08:38

Well, you can group the ones that have the same functionality:

public function typeCheck($type, $value)
{
    $this->value = $value;
    switch ($type) {
        case 'string':
            return is_string($value) || is_integer($value) || is_bool($value);
        case 'FortyStringNotNull':
            return is_string($value) && (strlen($value) < 41);
        case 'positiveInteger':
            return is_numeric($value) && ($value > 0);
        case 'boolean':
            return is_bool($value) || ($value == 'true' || $value = 'false');
        case 'float':
        case 'decimal':
        case 'integer':
            return is_numeric($value);
        case 'PositiveDimension':
            return is_numeric($value) && ($value > 0);
        case 'Dimension':
            return is_numeric($value) && (strlen($value) < 13);
        case 'Barcode':
            $validator = $this->getBarcodeValidator();
            $validator->setBarcode($value);
            return is_string($value) && (strlen($value) < 17 && $validator->isValid());
        case 'dateTime':
            return true;
        case 'normalizedString':
            $this->value = strip_tags($value);
            return is_string($value);
        case 'StringNotNull':
        case 'LongStringNotNull':
        case 'SuperLongStringNotNull':
        default:
            return is_string($value);
    }
}

That will reduce your CRAP index a little. However, if you are using OO, you should possibly think about using a Strategy pattern, have a look here for more info https://phpenthusiast.com/blog/strategy-pattern-the-power-of-interface

score 1 · Answer 3 · answered Sep 01 '20 at 12:44

To make your code more testable, create object methods out of each type check.


public function isTypeString($value) {
    return is_string($value);
}

public function isTypeLongStringNotNull($value) {
    return is_string($value);
}

Now you can call these methods 2 ways. You can pass $type to typeCheck() as before, or you can use some magic methods or call the isType...() methods directly

// class Whatever
 /**
 * Check type of attribute value
 * @param $type
 * @param $value
 * @return bool
 */
public function typeCheck($type, $value)
{
    $this->value = $value;
    if (method_exists($this, 'isType'.ucfirst($type)) {
       return call_user_func([$this, 'isType'.ucfirst($type)], $value);
    }
    return this->isTypeString($value);
}

public function __call($method, $args) {
   return call_user_func_array( [$this, 'isType'.ucfirst($type)], $args);
}
// } class



//3 ways to call the type check methods

//regular way
$x = new Whatever();
$x->typeCheck('LongStringNotNull', $val);
$x->isTypeLongStringNotNull($val);

//magic __call
$x->LongStringNotNull($val);

//string interpolation
$check = 'LongStringNotNull';
$x->{$check}($val);
$x->{'isType'.$check}($val);

Alternative for too many switch case for cyclomatic complexity reduction?

3 Answers3