PHP Script - Converting string of calculations to result

Question

So I am making a little script to convert a string of a basic maths calculation eg:

"2+3+4-2+2-12+42"

and it should return the total.

What I have so far :

<?php

function my_operator($a, $b, $char) {
    switch($char) {
        case '-': return $a - $b;
        case '+': return $a + $b;
        default: return $a;
    }
}

$string = "1+41-3+5-12+5+10";
//Ans = 47

$Arr = str_split($string);
$total = 0;
$cache1 = "";
$cache2 = "";
$lastOperator ="";

foreach ($Arr as $char) {
    if ($char == "+") {
        if ($cache2 == "") {
            $total = $cache1;
        }
        $cache2 = $cache1;
        $cache1 = "";
        $total = my_operator($total, (int)$cache2, $lastOperator);
        $lastOperator = "+";
    } else if ($char == "-") {
        if ($cache2 == "") {
            $total = $cache1;
        }
        $cache2 = $cache1;
        $cache1 = "";
        $total = my_operator($total, (int)$cache2, $lastOperator);
        $lastOperator = "-";
    } else {
        $cache1 = $cache1.$char;
    }
}

$total = my_operator($total, (int)$cache1, $lastOperator);

echo $total;

What are some improvements I can make? Thanks.

Answer 1

I really should be discussing your code here, but ones I got started I took it a bit too far. So before I do that, let's talk about your code.

names

Names of variables and functions give meaning to your code. They should tell the reader exactly what is in them, or what they do. A name like $string or $Arr clearly doesn't do that. Most names are good, but they all need to be good for the code to be easily read. You'll notice that readability is very important to me.

functions

You've clearly mastered the usage of functions, but you're not using them effectively. Functions allow you to write code with another level of abstraction and meaning, making it easier to understand. Functions also isolate variables from the global scope, which will become important when your application grows.

don't repeat yourself (DRY)

See: https://en.wikipedia.org/wiki/Don%27t_repeat_yourself

I've noticed the following structure in your code:

if ($char == "+") {
    .....
    $lastOperator = "+";
} else if ($char == "-") {
    .....
    $lastOperator = "-";
} else {
    $cache1 = $cache1.$char;
}

the ..... represent two pieces of code that are exactly the same. Why not write:

if (($char == "+") || ($char == "-")) {
    .....
    $lastOperator = $char;
} else {
    $cache1 = $cache1.$char;
}

that way you don't have to repeat the same code twice.

choice of algorithm

You process input characters one by one. This is hard. No really, it is. Would this really be the best algorithm? I don't think so. How would a person do the calculation? They would split the string like this: 1,+,41,-,3,+,5,-,12,+,5,+,10, and then simply perform the operations sequentially. Why don't you? With explode() you could split the string like this:

function splitQuestion($question)
// split a math question into numbers, plus and minus
{
  return explode('#',str_replace(['+','-'],['#+#','#-#'],$question));
}

// example code
$question = '1+41-3+5-12+5+10';
echo '<pre>'.print_r(splitQuestion($question),TRUE).'</pre>';

The trick here is to first insert the hashes and only then explode on that. After this writing an algorithm to do the calculation will be much easier because you don't need to cache any characters.

error checking

Your code almost seems to assume the input is valid. There's little checking of, and tolerance for, syntax errors in the input. A programmer should always think about possible errors in the input. They will always occur, and in my experience unexpected input is the most common 'cause' of bugs.

playtime

I couldn't resist to write a very short calculator for you:

// user input
$question = '1+41-3+5-12+5+10';
// get the answer
eval('$answer = '.$question.';');
// show question and answer
echo $question.' = '.$answer;

There are several reasons to be scared of the eval() function in PHP, read the warning here: http://php.net/manual/en/function.eval.php

This warning is appropriate and should be taken notice of. So, normally you really shouldn't use it, but in the case of a calculator it can be very powerful. You can basically do everything which PHP can do, and that's a lot (hence the danger).

As long as you hardcode the math question in PHP there's no problem, but that's not the point of a calculator, you want user input. So instead of working hard on deciphering the question, why not restrict and validate the user input and use eval() to calculate the result?

For instance if only the characters 1234567890+- are allowed in the question string nothing damaging can happen. This would only add one line to the code:

// user input
$question = '1+41-3+5-12+5+10';
// filter everything except numbers, plus and minus
$question = filter_var($question,FILTER_SANITIZE_NUMBER_INT);
// get the answer
eval('$answer = '.$question.';');
// show question and answer
echo $question.' = '.$answer;

You would want to put this code in a function for two reasons: It isolates the eval() even more, and it makes your code easier to read. Like so:

function calculate($question)
// perform a calculation with numbers, plus and minus
{
  return eval('return '.filter_var($question,FILTER_SANITIZE_NUMBER_INT).';');
}

// user input
$question = '1+41-3+5-12+5+10';
// show question and answer
echo $question.' = '.calculate($question);

This can be easily extended to multiplications and devision, or the use of brackets. Do with it what you want.