2
\$\begingroup\$

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();    
        }
    }    
}
?>
\$\endgroup\$
2
\$\begingroup\$

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!

\$\endgroup\$
  • \$\begingroup\$ I understand you meant it just as an example, but I'd just keep it as an injectable component, that way you wouldn't have to modify your class at all if you ever needed it in multiple places. \$\endgroup\$ – brgs Jun 18 '18 at 9:49
  • \$\begingroup\$ I totally agree. In OOP approach we should make all classes as reusable as possible, dependency injections will decouple objects. In my example i use classes more like in functional programming way. \$\endgroup\$ – Ivan Vartanyan Jun 18 '18 at 9:57
  • \$\begingroup\$ Thanks for the example. Consider that it's only an exercise and I didn't thought to an implementation with other classes. \$\endgroup\$ – user9741470 Jun 18 '18 at 11:21
4
\$\begingroup\$

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
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.