2
\$\begingroup\$

I mostly need feedback on the client-side JS.

Calculator.html is mostly a big button array made in Bootstrap.

!DOCTYPE html>
<html>
<head>
    <script src="https://code.jquery.com/jquery-3.2.1.min.js"></script>
    <script src="https://cdnjs.cloudflare.com/ajax/libs/popper.js/1.12.3/umd/popper.min.js"></script>
    <script src="https://maxcdn.bootstrapcdn.com/bootstrap/4.0.0-beta.2/js/bootstrap.min.js"></script>
    <script src="Evaluator.js"></script>
    <link rel="stylesheet" href="style.css">
    <link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/font-awesome/4.7.0/css/font-awesome.min.css">
    <link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/4.0.0-beta.2/css/bootstrap.min.css">
</head>
<body style="padding-left:10px;">
<div class="container">
    <div class="row">
        <div class="card card-info">
            <div class="card-header"><label>CTI110 Final Project</label></div>
            <div class="card-block"><input id="Expression" type="text" placeholder="2+2=4" class="form-control" style="text-align: right;"></div>
            <div role="toolbar" class="btn-toolbar">
                <div role="group" class="btn-group-lg"><button id="Backspace" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">&#8592;</button></div>
                <div role="group" class="btn-group-lg"><button id="Clearentry" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">CE</button></div>
                <div role="group" class="btn-group-lg"><button id="Clearall" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">C</button></div>
                <div role="group" class="btn-group-lg"><button id="Plusminus" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">±</button></div>
                <div role="group" class="btn-group-lg"><button id="Max" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">⋀</button></div>
            </div>
            <div role="toolbar" class="btn-toolbar">
                <div role="group" class="btn-group-lg"><button id="Seven" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">7</button></div>
                <div role="group" class="btn-group-lg"><button id="Eight" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">8</button></div>
                <div role="group" class="btn-group-lg"><button id="Nine" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">9</button></div>
                <div role="group" class="btn-group-lg"><button id="Divide" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">/</button></div>
                <div role="group" class="btn-group-lg"><button id="Min" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">∨</button></div>
            </div>
            <div role="toolbar" class="btn-toolbar">
                <div role="group" class="btn-group-lg"><button id="Four" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">4</button></div>
                <div role="group" class="btn-group-lg"><button id="Five" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">5</button></div>
                <div role="group" class="btn-group-lg"><button id="Six" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">6</button></div>
                <div role="group" class="btn-group-lg"><button id="Multiply" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">*</button></div>
                <div role="group" class="btn-group-lg"><button id="Modulus" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">%</button></div>
            </div>
            <div role="toolbar" class="btn-toolbar">
                <div role="group" class="btn-group-lg"><button id="One" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">1</button></div>
                <div role="group" class="btn-group-lg"><button id="Two" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">2</button></div>
                <div role="group" class="btn-group-lg"><button id="Three" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">3</button></div>
                <div role="group" class="btn-group-lg"><button id="Subtract" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">-</button></div>
                <div role="group" class="btn-group-lg"><button id="^" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">x<sup>x</sup></button></div>
            </div>
            <div role="toolbar" class="btn-toolbar">
                <div role="group" class="btn-group-lg"><button id="Zero" type="button" style="width: 118px; height: 59px" class="btn btn-secondary rounded-0">0</button></div>
                <div role="group" class="btn-group-lg"><button id="Decimal" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">.</button></div>
                <div role="group" class="btn-group-lg"><button id="Add" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">+</button></div>
                <div role="group" class="btn-group-lg"><button id="Equals" type="button" style="width: 59px; height: 59px" class="btn btn-secondary rounded-0">=</button></div>
            </div>
        </div>
    </div>
</div>
</body>
</html>

Evaluator.js

Basically this is just responsible for the building of the expression, doing some basic checks, and sending it to the server. This is the part that is pretty sloppy and I think there is a better way of doing it.

function Expression() {
    this.expression = "";
    this.OperatorCheck = false;
}
Expression.prototype.ToString = function() {
    return this.expression;
};
Expression.prototype.Clear = function() {
    this.expression = "";
    this.OperatorCheck = false;
    $("#Expression").empty();
};
Expression.prototype.InsertNumber = function(Expression) {
    this.expression += Expression;
};
Expression.prototype.InsertOperator = function(Operator) {
    if (!this.OperatorCheck) {
        this.expression += " " + Operator.toString() + " ";
        this.OperatorCheck = true;
    } else {
        alert("Only one operator allowed!");
    }
};

Expression.prototype.Backspace = function() {
    this.expression = this.expression.slice(0,-1);
    if (this.expression.length === 0) {
        this.Clear();
    }
};
Expression.prototype.Submit = function() {
    let Data = {
        "IntegerA":  this.expression.substr(0,this.expression.indexOf(' ')),
        "Operation": this.expression.substr(this.expression.indexOf(' ')+ 1,1),
        "IntegerB":  this.expression.substr(this.expression.indexOf(' ')+ 3)
    };

    $.ajax({
        url:"Math.php", dataType: "json", data: Data, method: "POST",
        success: $.proxy(function(response) {
            $("#Expression").val(response['Result']);
            this.Clear();
        }).bind(this)
    });
};

$(document).ready(function() {
    let exp = new Expression();
    $(document).click(function(e) {
        let buttonText = $(e).attr('target').innerHTML;

        if (!(buttonText.match(/[^.\d]/)))
            exp.InsertNumber(buttonText);
        else if (buttonText === "=")
            exp.Submit();
        else if (buttonText === "CE" || buttonText === "C")
            exp.Clear();
        else if (buttonText === "←")
            exp.Backspace();
        else if (buttonText === "±")
            alert("Not currently supported");
        else if (buttonText === "x<sup>x</sup>")
            exp.InsertOperator("^");
        else
            exp.InsertOperator(buttonText);

        $("#Expression").val(exp.ToString());
    });
});

Evaluator.php is just a bunch of static anonymous functions.

<?php

$Add = static function($IntA,$IntB) {
    return $IntA + $IntB;
};

$Sub = static function($IntA,$IntB) {
    return $IntA - $IntB;
};

$Div = static function($IntA,$IntB) {
    return $IntA / $IntB;
};

$Mul = static function($IntA,$IntB) {
    return $IntA * $IntB;
};

$Pow = static function($IntA,$IntB) {
    return pow($IntA,$IntB);
};

$Mod = static function($IntA,$IntB) {
    return $IntA % $IntB;
};

$Min = static function($IntA,$IntB) {
    return min($IntA,$IntB);
};

$Max = static function($IntA,$IntB) {
    return max($IntA,$IntB);
};

Math.php just makes a map of the functions to the operations. I thought it was clever and I hate long if chains.

<?php
include 'Evaluator.php';

if (empty($_POST)) {
    $_POST = json_decode(file_get_contents("php://input"), true) ? : [];
}

function json_response($message = null, $code = 200) {
    header_remove();
    http_response_code($code);
    header("Cache-Control: no-transform,public,max-age=300,s-maxage=900");
    header('Content-Type: application/json');
    $status = array(200 => '200 OK', 400 => '400 Bad Request', 422 => 'Unprocessable Entity', 500 => '500 Internal Server Error');
    header('Status: '.$code);
    return json_encode(array('status' => $code < 300,'Result' => $message));
}


$IntegerA = isset($_POST["IntegerA"]) ? $_POST["IntegerA"] : 0;
$IntegerB = isset($_POST["IntegerB"]) ? $_POST["IntegerB"] : 0;
$Operation = isset($_POST["Operation"]) ? $_POST["Operation"] : "";


$OperationMap = Array();
$OperationMap['+'] = $Add;$OperationMap['-'] = $Sub;$OperationMap['/'] = $Div;$OperationMap['*'] = $Mul;
$OperationMap['^'] = $Pow;$OperationMap['%'] = $Mod;$OperationMap['⋀'] = $Max;$OperationMap['∨'] = $Min;

foreach($OperationMap as $key => $value) {
    if ($key == $Operation) {
        echo json_response($value($IntegerA,$IntegerB),200);
    }
}
\$\endgroup\$
  • \$\begingroup\$ 4 code blocks... meh.. that's too much for me to read right now, just wanna say that it seems like you're going to pretty extreme lengths to avoid using eval just because of some stigma that it's unsafe.. there's a way to safely use eval and doing so will hugely reduce your code base and maek your calculator faster.. not a popular opinion but it's true. \$\endgroup\$ – I wrestled a bear once. Dec 8 '17 at 19:38
  • 1
    \$\begingroup\$ I already used eval on a previous assignment and got a 0 :/. Teachers rules, not mine. \$\endgroup\$ – Jamison Sasser Dec 9 '17 at 0:59
  • \$\begingroup\$ fair enough, good to know how to avoid it \$\endgroup\$ – I wrestled a bear once. Dec 9 '17 at 1:04
3
\$\begingroup\$

Feedback

I like the usage of an event delegate in the JavaScript code - it is nice how the click event is bound once. Also, the event handling code is separated from the markup - great job!

Suggestions

Javascript

Data format

Instead of using substrings to separate the operands and operator, String.split() could be used to separate the expression into an array of pieces. Actually, maybe it would be wise to store the expression using that data structure and display it by by calling Array.join() on the array from calling Object.values().

Cache DOM references

There are multiple places where $('#Expression') exists - that could be stored in a variable when the DOM is ready (e.g. passed to the Expression constructor function, which would store it on this) and then that variable referenced instead of fetching it each time.

if chains (Javascript)

You mentioned (when discussing the PHP code) that you don't like long if chains. But it appears that there is a decent if chain in the Javascript code that delegates function calls based on the button that was clicked. You could likely simplify that by creating a mapping of buttonText values to functions of Expression

let vs const

In the function Expression.prototype.Submit, there is a variable Data declared using let. It appears that value doesn't change within the lifetime of that function. Why not use const instead? The same applies to buttonText in the click handler.

jQuery wrapper on event

The line:

let buttonText = $(e).attr('target').innerHTML;

Could be simplified to merely:

const buttonText = e.target.innerHTML;

Because there isn't much need in passing the event object in the jQuery wrapper just to get the innerHTML of the target (and as mentioned above, the value doesn't appear to get re-assigned so const can be used).

For more context, refer to the article below. I know it comes off in the beginning as tough on jQuery but it has some useful information. Stop writing Slow Javascript.

PHP

Unused variable: $status

It appears that variable $status is unused. Was that used for debugging, or checking the value of $code?

Mapping built-in functions

The static anonymous that call built-in functions (e.g. min(), max(), pow()) could just be replaced by the name of the function in a string literal.

$OperationMap['^'] = 'pow';
$OperationMap['⋀'] = 'max';
$OperationMap['∨'] = 'min';
\$\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.