PHP OOP calculator form

Question

I aim to improve my PHP OOP skills and made a simple calculator. Visitors can choose 2 numbers and a prefix, so they can add, subtract, divide and so forth. The output is the sum. The good thing is it's working, but is it also clean coding? Also, is it wise to use the switch statement in a function?

I hope to get some tips so I can improve and learn.

class calcdata {

public $number1;
public $prefix;
public $number2;

public function __construct() {
    $this->number1 = $number1 = $_POST['number1'];
    $this->prefix = $prefix = $_POST['prefix'];
    $this->number2 = $number2 = $_POST['number2'];


}

public function result(){

switch($this->prefix){
case "+":
return ($this->number1 + $this->number2);
break;
case "-":
return ($this->number1 - $this->number2);
break;
case "/":
return ($this->number1 / $this->number2);
break;
case "*":
return ($this->number1 * $this->number2);
break;
default:
return "Only use: \" +, -, / of * \" ";
}
}
}

$show = new calcdata();
echo $show->result();


    </body>
 </html>

Answer 1

Constructor

I wouldn't extract the POST values inside the constructor. For the calculator class, it shouldn't matter where the values come from (it could be POST, GET, the database, anywhere really).

I would just pass the arguments as variables like this:

public function __construct($number1, $number2, $prefix) {
    $this->number1 = $number1;
    $this->number2 = $number2;
    $this->prefix = $prefix;
}

Your constructor also defined $number1 etc without using it. If you want to stay with your approach, the assignments should look like this: $this->number1 = $_POST['number1'];

Input Checking

Right now, you just accept anything the user passes. For example:

prefix=*&number2=0xf&number1=0xd
-> 195
prefix=-&number2=5
-> -5
prefix=*
-> 0
prefix=/
-> error (because of the division by 0)

If this is your intention, then that is fine, but otherwise you should check if all values are set and if they are all decimal numbers. At the least I would check for division by 0.

Misc

• I would call prefix command or operation.
• it is customary to use CamelCase for class names, so 'calcdata' could be 'CalcData' (or something a bit more concrete like BasicCalculator).
• your indentation is off. Any IDE can fix this, and it makes your code more readable.
• you have unnecessary newlines (which makes your code a bit harder to read).

is it wise to use the switch statement in a function?

In this case it's fine, but if you plan to create a more complex calculator class, I would just create public add, divide, negate (just one argument), log (three arguments), etc. methods, because a calculator should only have to calculate, it shouldn't actually be the calculators job to decide what it calculates. And it would be hard to manage once you have functions with more or less than two arguments.

So in that case I would extract the switch statement to a separate controller class which then decides what to calculate based on the user input.

Answer 2

To be honest, this code is only a switch statement. I would recommend you to sanitize the $_POST before saving it to $this. Shouldent have anything to say but if it was connected to a database or the $_POST was saved and displayed again, that would be a problem.