2

How can I reduce the cyclomatic complexity of a function that returns a value dependent on the cartesian product of 3 booleans? How can I make the following code look more clean?

This is for a school project, and is not really a requirement for the assignment itself, but I usually find myself writing functions that rely on a - sometimes - rather complex truth table. I'm thinking that this cannot be the best way to do it.

    public function getDiscount( $values ) {

    $res       = new stdClass();
    $res->code = 400;

    if ( ! is_bool( $values['new_customer'] ) || ! is_bool( $values['loyalty_card'] ) || ! is_bool( $values['coupon'] ) ) {
        $res->data = "Missing inputs";

        return $res;
    }

    if ( $values['new_customer'] == true && $values['loyalty_card'] == true && $values['coupon'] == true ) {
        $res->data = "Invalid input";

        return $res;
    }

    if ( $values['new_customer'] == true && $values['loyalty_card'] == true && $values['coupon'] == false ) {
        $res->data = "Invalid input";

        return $res;
    }

    $res->code = 200;

    if ( $values['new_customer'] == true && $values['loyalty_card'] == false && $values['coupon'] == true ) {
        $res->data = 20;

        return $res;
    }

    if ( $values['new_customer'] == true && $values['loyalty_card'] == false && $values['coupon'] == false ) {
        $res->data = 15;

        return $res;
    }

    if ( $values['new_customer'] == false && $values['loyalty_card'] == true && $values['coupon'] == true ) {
        $res->data = 30;

        return $res;
    }

    if ( $values['new_customer'] == false && $values['loyalty_card'] == true && $values['coupon'] == false ) {
        $res->data = 10;

        return $res;
    }

    if ( $values['new_customer'] == false && $values['loyalty_card'] == false && $values['coupon'] == true ) {
        $res->data = 20;

        return $res;
    }

    if ( $values['new_customer'] == false && $values['loyalty_card'] == false && $values['coupon'] == false ) {
        $res->data = 0;

        return $res;
    }

    $res->code = 400;
    $res->data = "Invalid input";
    return $res;

}
Emil Rosenius
  • 961
  • 1
  • 9
  • 24
  • You are right, that code is way to verbose for what you need, save me some trouble through, please edit your question to include your truth table so that we don't get the assumptions wrong about your logic. – Chris Schaller Mar 21 '17 at 00:54

1 Answers1

4

This is a language agnostic paradigm and you will find that different languages will have different inbuilt functions and operators to facilitate processing of truth tables.

My Favourite is the new pattern matching in C# 7.0 switch statements https://visualstudiomagazine.com/articles/2017/02/01/pattern-matching.aspx

First things first, identify your truth table

CASE    New Customer    Loyalty     Coupon  Output
  1        Yes            Yes        Yes    'Invalid Input'
  2        Yes            Yes        No     'Invalid Input'             
  3        Yes            No         Yes      20
  4        Yes            No         No       15                
  5        No             Yes        Yes      30
  6        No             Yes        No       10                
  7        No             No         Yes      20
  8        No             No         No       0             

Immediately we can see that case 1 and 2 are not dependant on the coupon condition, so that is your first optimisation. The second condition to notice is that there are no cases where new customer is true and they have a loyalty card, this makes sense. From there, the simplest to both follow visually in code and to program is to use nested branching, that way we only evaluate each condition check once.

public function getDiscount( $values ) {

    $res       = new stdClass();
    $res->code = 400;

    if ( ! is_bool( $values['new_customer'] ) || ! is_bool( $values['loyalty_card'] ) || ! is_bool( $values['coupon'] ) ) {
        $res->data = "Missing inputs";

        return $res;
    }

    if ( $values['new_customer'] == true && $values['loyalty_card'] == true ) {
        $res->data = "Invalid input";

        return $res;
    }

    $res->code = 200;

    // Check New Customer conditions
    if ( $values['new_customer'] == true) {
        if( $values['coupon'] == true ) 
            $res->data = 20;
        else
            $res->data = 15;
    }

    // Check existing customer conditions
    else {

        // Has Loyalty Card
        if ( $values['loyalty_card'] == true ) {
            // Has Coupon
            if( $values['coupon'] == true ) 
                $res->data = 30;
            else
                $res->data = 10;
        }

        // No Loyalty Card
        else {
            // Has Coupon
            if( $values['coupon'] == true ) 
                $res->data = 20;
            else
                $res->data = 0;
        }
    }

    // Don't need a default fail condition here, because we have covered all possible combinations     
    return $res;

}

It might have also have been easier to use value addition, the truth table is a good way to represent the next block of code:

CASE    New Customer    Loyalty     Coupon  Output
  1        Yes            Yes        Yes    'Invalid Input'
  2        Yes            Yes        No     'Invalid Input'             
  3        Yes  +15       No         Yes +5   =20
  4        Yes  +15       No         No       =15               
  5        No             Yes +10    Yes +20  =30
  6        No             Yes +10    No       =10               
  7        No             No         Yes +20  =20
  8        No             No         No       0             

public function getDiscount( $values ) {

    $res       = new stdClass();
    $res->code = 400;

    if ( ! is_bool( $values['new_customer'] ) || ! is_bool( $values['loyalty_card'] ) || ! is_bool( $values['coupon'] ) ) {
        $res->data = "Missing inputs";

        return $res;
    }

    if ( $values['new_customer'] == true && $values['loyalty_card'] == true ) {
        $res->data = "Invalid input";

        return $res;
    }

    $res->code = 200;
    $res->data = 0;

    // Check New Customer conditions
    if ( $values['new_customer'] == true) {
        $res->data += 15;
        if( $values['coupon'] == true ) 
            $res->data += 5;
    }

    // Check existing customer conditions
    else {

        // Has Loyalty Card
        if ( $values['loyalty_card'] == true )
            $res->data += 10;

        // Has Coupon
        if( $values['coupon'] == true ) 
            $res->data += 20;
    }

    // Don't need a default fail condition here, because we have covered all possible combinations     
    return $res;

}

There are many ways to skin this cat :) The above code examples highlight how you can use branching logic to only evaluate each condition once. While trivial in this scenario, evaluating some conditions in the future may have heavy performance implications, so you would only want to evaluate once.

Chris Schaller
  • 13,704
  • 3
  • 43
  • 81