Simple basic calculator php class

Question

I wrote this basic calculator class using php.It will only execute the basic arithmetics operations so any suggestion for new functions add and improvements will be accepted. Thank you.

<?php

class Calculator{

public $x , $y , $z;    

    public function __construct($x,$y,$z){
        $this->x = $x;
        $this->y = $y;
        $this->z = $z;
    }

    public function calc(){
        try{
            if($this->x != 0 && $this->z != 0){

                switch($this->y){
                case '+': 
                return ($this->x + $this->z); 
                break;

                case '-':
                return ($this->x - $this->z);
                break;

                case '/':
                return ($this->x / $this->z);
                break;    

                case '*':
                return ($this->x * $this->z);
                break;

                } 
            } else {

                throw new Exception('Operations with 0 not alloewd!'); 

            }    
        } 
        catch(Exception $e){
            return $e->getMessage();    
        }
    }    
}
?>

Answer 1

Completely agree with Dan and burgeris on their comments. Just want to sum up everything and improve the solution.

First of all I have to notice that your code is not object-oriented, because it is not manipulating objects. In fact you're not creating any proper objects at all. If you want to develop it using OOP, you need to create at least 3 classes: Operand, Operator, Calculator. It is good to try it for educational purposes, so start google a good articles about OOP.

This task can also be done by using functions or by creating utility class. Let's try utility class approach. For this task it is natural for me to use static methods, so the code will look like this:

<?php

/**
 * Calculator. Not object-oriented utility class
 */
class Calculator {

    /**
     * Calculates the result of operation between two operands
     *
     * @param int|float $operand1 First operand
     * @param string $operation Operation to cast
     * @param int|float $operand2 Second operand
     * @return int|float
     * @throws \Exception
     */
    public static function calculate($operand1, $operation, $operand2) {
        // Here should be a code for checking method parameter types

        if ($operand1 == 0 && $operand2 == 0) {
            throw new Exception('Operations with 0 not allowed!');             
        }

        // Choose operation and calculate the value
        switch($operation) {
            case '+': 
                return ($operand1 + $operand2); 
                break;

            case '-':
                return ($operand1 - $operand2);
                break;

            case '*':
                return ($operand1 * $operand2);
                break;

            case '/':
                return ($operand1 / $operand2);
                break;    

            default:
                throw new \Exception('Unknown operator');
                break;
        }
    }
}

// Let's try to calculate something
echo Calculator::calculate(42, '*', 1);

How does this look like? For me code is: readable, well documented, easy to use. Variables has a proper names, comments are everywhere :) Good luck in learning!

Answer 2

Im not going to comment on the implementation of the calculator (there are more well versed people out there) but I will comment on the coding style;

Return early (or in this case throw early)

I much prefer (I think most people do) to return or throw early to avoid more brackets and indentation.

So I would change the body of calc() to look like;

if($this->x == 0 && $this->z == 0){
    throw new \Exception('Operations with 0 not allowed!');             
}

switch($this->y){
    case '+': 
    return ($this->x + $this->z); 
    break;

    case '-':
    return ($this->x - $this->z);
    break;

    case '/':
    return ($this->x / $this->z);
    break;    

    case '*':
    return ($this->x * $this->z);
    break;

    default: 
    throw new \Exception('Unknown operator');
    break;

}

Let Exceptions bubble up

If this is a calculator the excepted return is a number, not a string so doing the whole try catch and returning the $e->getMessage() is probably not what the developer expects to happen.

Variables In The Constructor

There is a lot of resources out there that suggest this is the correct way to pass data to a class.

To me variables passed to a constructor should be other classes this class depends on, the reason is so we don't have to add more manual definitions when using dependency injection (this is not true in every instance).

but here are some arguments why in this particular instance I wouldn't pass data to the constructor

• For dependency injection you are going to have to define where those variables are
• You will have to instantiate a new Calculator(x, y, z) every time you want to use it which seems wasteful as you return the values and not store them for another class to use

Answer 3

First thing that caught my eye was meaningless variable names: x, y, z. Although the class is extremely simple, it wouldn't hurt giving them proper names like operand, operator, etc.