Reputation: 4390

Confused about a simple switch in PHP (5.5.38)

I have setup the following class which mimics one I am currently working on and trying to fix.

The goal

To return a fee (value) based on an initial value that is passed in.

The issues

I have two issues with the current code below:

When the first value that is passed in is 0 (zero) 12p is returned which is incorrect - it should be caught by the first case and return 15p.
I am yet to tackle it but I want to return the issuance fee for the last case that is satisfied (if the next ones are not defined). For the example below; the last restriction to be set is the $lower_3 variable - which means that any value passed in that is greater than 31* should return a fee of 10p. I am however seeing that null is being returned as the checks just fall through to the last case.

The code

<?php

class IssuanceScheme
{
    private $lower_1 = 0;
    private $upper_1 = 20;
    private $issuanceFee_1 = '15p';

    private $lower_2 = 21;
    private $upper_2 = 30;
    private $issuanceFee_2 = '12p';

    private $lower_3 = 31;
    private $upper_3 = null; //50;
    private $issuanceFee_3 = '10p';

    private $lower_4 = null; //51;
    private $upper_4 = null; //75;
    private $issuanceFee_4 = null;

    private $lower_5 = null; //76;
    // no $upper_5
    private $issuanceFee_5 = null;

    public function calculateIssuanceFee($volume)
    {
        $issuanceFee = $this->issuanceFee_1;

        switch ($volume) {
            case ($volume >= $this->lower_1 && $volume <= $this->upper_1):
                $issuanceFee = $this->issuanceFee_1;
                break;
            case ($volume >= $this->lower_2 && $volume <= $this->upper_2):
                $issuanceFee = $this->issuanceFee_2;
                break;
            case ($volume >= $this->lower_3 && $volume <= $this->upper_3):
                $issuanceFee = $this->issuanceFee_3;
                break;
            case ($volume >= $this->lower_4 && $volume <= $this->upper_4):
                $issuanceFee = $this->issuanceFee_4;
                break;
            case ($volume >= $this->lower_5):
                $issuanceFee = $this->issuanceFee_5;
                break;
        }

        return $issuanceFee;
    }
}

$issuanceScheme = new IssuanceScheme;

// 15p
echo sprintf("Passed in: %s, Exp: %s, Got: %s", 0, '15p', $issuanceScheme->calculateIssuanceFee(0));
echo sprintf("Passed in: %s, Exp: %s, Got: %s", 5, '15p', $issuanceScheme->calculateIssuanceFee(5));
echo sprintf("Passed in: %s, Exp: %s, Got: %s", 10, '15p', $issuanceScheme->calculateIssuanceFee(10));
echo sprintf("Passed in: %s, Exp: %s, Got: %s", 15, '15p', $issuanceScheme->calculateIssuanceFee(15));
echo sprintf("Passed in: %s, Exp: %s, Got: %s", 20, '15p', $issuanceScheme->calculateIssuanceFee(20));

// 12p
echo sprintf("Passed in: %s, Exp: %s, Got: %s", 25, '12p', $issuanceScheme->calculateIssuanceFee(25));
echo sprintf("Passed in: %s, Exp: %s, Got: %s", 30, '12p', $issuanceScheme->calculateIssuanceFee(30));

// 10p
echo sprintf("Passed in: %s, Exp: %s, Got: %s", 50, '10p', $issuanceScheme->calculateIssuanceFee(50));
echo sprintf("Passed in: %s, Exp: %s, Got: %s", 60, '10p', $issuanceScheme->calculateIssuanceFee(60));
echo sprintf("Passed in: %s, Exp: %s, Got: %s", 75, '10p', $issuanceScheme->calculateIssuanceFee(75));
echo sprintf("Passed in: %s, Exp: %s, Got: %s", 100, '10p', $issuanceScheme->calculateIssuanceFee(100));

The result

Passed in: 0, Exp: 15p, Got: 12p // Error, expected is 15p
Passed in: 5, Exp: 15p, Got: 15p
Passed in: 10, Exp: 15p, Got: 15p
Passed in: 15, Exp: 15p, Got: 15p
Passed in: 20, Exp: 15p, Got: 15p
Passed in: 25, Exp: 12p, Got: 12p
Passed in: 30, Exp: 12p, Got: 12p
Passed in: 50, Exp: 10p, Got: // Not implemented (see goal 2, and not below)
Passed in: 60, Exp: 10p, Got:
Passed in: 75, Exp: 10p, Got:
Passed in: 100, Exp: 10p, Got:

The second goal hasn't been implemented but I want to return the last issuance fee that was satisfied. In this case 10p should be returned as the values passed in are greater than 31.

Upvotes: 1

Answers (3)

Jakub Matczak

Reputation: 15656

You misunderstand switch statement

switch($var) {
    case $val:
        //something

Means

if $var == $val then do //something

So in your code it's:

switch ($volume) {
    case ($volume >= $this->lower_1 && $volume <= $this->upper_1):
        $issuanceFee = $this->issuanceFee_1;
        break;

which means

if $volume == ($volume >= $this->lower_1 && $volume <= $this->upper_1) then do $issuanceFee = $this->issuanceFee_1;

Which is not true for $volume=0, because ($volume >= $this->lower_1 && $volume <= $this->upper_1) is true, so you get condition $volume == true which is 0 == true.

You need to reorganize your code probably into if {} elseif {} else {} statements.

For the second issue, you could change $volume <= $this->upper_1 to ($this->upper_1===null || $volume <= $this->upper_1), and the same for other levels.

That will skip upper limit check if it's set to null (which means something like unlimited).

public function calculateIssuanceFee($volume)
{
    $issuanceFee = $this->issuanceFee_1;
    if ($volume >= $this->lower_1 && ($this->upper_1===null || $volume <= $this->upper_1)) {
        return $this->issuanceFee_1;
    }
    if ($volume >= $this->lower_2 && ($this->upper_2===null || $volume <= $this->upper_2)) {
        return $this->issuanceFee_2;
    }
    if ($volume >= $this->lower_3 && ($this->upper_3===null || $volume <= $this->upper_3)) {
        return $this->issuanceFee_3;
    }
    if ($volume >= $this->lower_4 && ($this->upper_4===null || $volume <= $this->upper_4)) {
        return $this->issuanceFee_4;
    }
    if ($volume >= $this->lower_5) {
        return $this->issuanceFee_5;
    }

}

Here's working example: https://3v4l.org/uamib

Passed in: 0, Exp: 15p, Got: 15p 
Passed in: 5, Exp: 15p, Got: 15p 
Passed in: 10, Exp: 15p, Got: 15p 
Passed in: 15, Exp: 15p, Got: 15p 
Passed in: 20, Exp: 15p, Got: 15p 
Passed in: 25, Exp: 12p, Got: 12p 
Passed in: 30, Exp: 12p, Got: 12p 
Passed in: 50, Exp: 10p, Got: 10p 
Passed in: 60, Exp: 10p, Got: 10p 
Passed in: 75, Exp: 10p, Got: 10p 
Passed in: 100, Exp: 10p, Got: 10p

Upvotes: 5

Ede

Reputation: 397

Your use of the switch case is wrong. Whatever comes after case should be a value that matches whatever goes in switch(). An expression like $volume > $lower will be evaluated to a boolean, and this boolean will be compared with your $volume variable that goes in your switch($volume).

I'd advise to just use if statements with early returns:

public function calculateIssuanceFee($volume)
{
    if ($volume >= $this->lower_1 && $volume <= $this->upper_1) {
        return $this->issuanceFee_1;
    }
    if ($volume >= $this->lower_2 && $volume <= $this->upper_2) {
        return $this->issuanceFee_2;
    }
    if ($volume >= $this->lower_3 && $volume <= $this->upper_3) {
        return $this->issuanceFee_3;
    }
    if ($volume >= $this->lower_4 && $volume <= $this->upper_4) {
        return $this->issuanceFee_4;
    }
    if ($volume >= $this->lower_5) {
        return $this->issuanceFee_5;
    }

    return $this->issuanceFee_1;
}

Edit: looking at your code again, what you probably really want is define a list of fees with their lower and upper bounds, and return a default fee if none match. Your code could be much more compact by specifying it like this:

class IssuanceScheme
{
    private $fees = [
        ['min' => 0, 'max' => 20, 'fee' => '15p'],   
        ['min' => 21, 'max' => 30, 'fee' => '12p'],   
    ];
    private $defaultFee = '10p';

    public function calculateIssuanceFee($volume)
    {
        foreach ($this->fees as $fee) {
            if ($volume >= $fee['min'] && $volume <= $fee['max']) {
                return $fee['fee'];
            }
        }
        return $this->defaultFee;
    }
}

Upvotes: 2

Michael