I have setup the following class which mimics one I am currently working on and trying to fix.
To return a fee (value) based on an initial value that is passed in.
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.
<?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));
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.
You misunderstand switch statement
switch($var) {
case $val:
//something
Means
if
$var == $valthen 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
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;
}
}
Based on both your answers, I have switched the order and removed the upper bounds as I think this makes more sense. Please correct me if I am wrong, but it satisfies various values I am passing in.
public function calculateIssuanceFee($volume)
{
if (isset($this->lower_5) && $volume >= $this->lower_5) {
return $this->issuanceFee_5;
}
if (isset($this->lower_4) && $volume >= $this->lower_4) {
return $this->issuanceFee_4;
}
if (isset($this->lower_3) && $volume >= $this->lower_3) {
return $this->issuanceFee_3;
}
if (isset($this->lower_2) && $volume >= $this->lower_2) {
return $this->issuanceFee_2;
}
if (isset($this->lower_1) && $volume >= $this->lower_1) {
return $this->issuanceFee_1;
}
}