Basic arithmetic calculator with float, parentheses and operator priority support

Question

Summary: The basic calculator which allows the end-user to calculate the value by evaluating the math expression string. It supports all the main arithmetic operators, also extended by modulo and exponent operations. Evaluation does follow the classic arithmetic rules and precedence and does take parentheses into consideration.

Note: While it detects a bunch of errors on the end-user side (such as division by zero or incorrect parentheses), most of them are left unnoticed. The user input of "----1" is still considered valid. The iterator, which presumably parses the integer/float, do not detect forbidden characters, so "5.0g" is also valid. To add, I could have used the istringstream for parsing values, but chose to do it manually as an exercise and wish to leave it as is.

Would like to hear the tips.

#include <iostream>
#include <vector>
#include <unordered_map>
#include <string>
#include <string_view>
#include <algorithm>
#include <stack>
#include <memory>
#include <cmath>
#include <cctype>
#include <regex>

class Calculator {
private:
    Calculator() = default;
    Calculator(std::string const& str) : _expr(str) {
        _expr.erase(std::remove_if(std::begin(_expr), std::end(_expr), [&](auto ch) -> bool { return std::isspace(ch); }), std::end(_expr));
        try {
            std::stack<std::string::value_type> p_stack;
            for (const auto& ch : _expr) {
                switch (ch) {
                case '(': p_stack.push('('); break;
                case ')': if (p_stack.empty() || p_stack.top() != '(')
                    throw std::runtime_error("error: unbalanced or unexpected parentheses\n");
                          else
                    p_stack.pop(); break;
                default: break;
                }
            }
            if (!p_stack.empty())
                throw std::runtime_error("error: unbalanced or unexpected parentheses\n");
            fretval = evaluate();
            std::cout << fretval << "\n";
        }
        catch (std::exception const& e) { std::cerr << e.what() << "\n"; }
    }

    enum class enum_op_t : char
    { OP_ADD = '+', OP_SUB = '-', OP_EXP = '^', OP_MUL = '*', OP_DIV = '/', OP_MOD = '%', NULLTYPE = '0' };

    class operator_t {
    public:
        enum_op_t      _type;         
        unsigned short _priority = 0; 
        char           _assoc;        
        operator_t(enum_op_t const& s, unsigned short i = 0, char a = 'L')
            : _type(s), _priority(i), _assoc(a) {}
    private:
        operator_t() = delete;
    };

    Calculator(Calculator const&) = delete;
    Calculator(Calculator&&) = delete;
    auto operator=(Calculator const&) = delete;

    std::string           _expr;
    std::string::iterator _cur = std::begin(_expr);
    std::stack<std::pair<operator_t, double>> mid;
    double fretval;

    auto getOperator() {
        switch (*_cur) {
            case '+': _cur++; return operator_t(enum_op_t::OP_ADD, 1, 'L');
            case '-': _cur++; return operator_t(enum_op_t::OP_SUB, 1, 'L');
            case '*': _cur++; return operator_t(enum_op_t::OP_MUL, 2, 'L');
            case '/': _cur++; return operator_t(enum_op_t::OP_DIV, 2, 'L');
            case '^': _cur++; return operator_t(enum_op_t::OP_EXP, 3, 'R');
            case '%': _cur++; return operator_t(enum_op_t::OP_MOD, 2, 'L');
            default:          return operator_t(enum_op_t::NULLTYPE);       break;
        }
    }

    auto getNumerical() {
            std::string::iterator _tmp = _cur;
            for (; std::isdigit(*(_tmp)) || *_tmp == '.'; ++_tmp) {}
            std::string tstr = std::string(_cur, _tmp);
            if (!std::regex_match(tstr, std::regex {"[+-]?([0-9]*[.])?[0-9]+"})) {
                throw std::runtime_error("error: could not parse token, expect <int> or <float>.\n");
            }
            auto retval = std::stod(tstr);
            _cur = _tmp;
            return retval;
    };

    auto performOp(operator_t var, double lhs, double rhs) {
            switch (var._type) {
            case enum_op_t::OP_ADD: return lhs + rhs;
            case enum_op_t::OP_DIV: if (!rhs) {
                                        throw std::runtime_error("error: division by zero.\n");
                                    }
                                    else
                                        return lhs / rhs;
            case enum_op_t::OP_MUL: return lhs * rhs;
            case enum_op_t::OP_SUB: return lhs - rhs;
            case enum_op_t::OP_MOD: if (!rhs) {
                                        throw std::runtime_error("error: mod 0 is forbidden.\n");
                                    }
                                    else
                                        return std::fmod(lhs, rhs);
            case enum_op_t::OP_EXP: return std::pow(lhs, rhs);
            default: return 0.0;
        }
    }

    double getValue() {
            double retval = 0;
            switch (*_cur) {
                case '0': case '1': case '2': case '3': case '4': case '5':
                case '6': case '7': case '8': case '9':
                    retval = getNumerical(); break;
                case '(':
                    _cur++; retval = evaluate();
                    if (*_cur != ')') {
                        if (_cur <= std::end(_expr))
                            throw std::runtime_error("error");
                        throw std::runtime_error("error: value expected, got token.\n");
                    }
                    _cur++; break;
                case '+': _cur++; retval = getValue();        break;
                case '-': _cur++; retval = getValue() * (-1); break;
                default: throw std::runtime_error("error: unexpected expression, could not parse.\n");

            }
            return retval;
    }

    double evaluate() {
        mid.push({ operator_t {enum_op_t::NULLTYPE}, 0 });
        double retval = getValue();
        while (!mid.empty()) {
            operator_t var { getOperator() };
            while (var._priority < mid.top().first._priority || (var._priority == mid.top().first._priority && var._assoc == 'L')) 
            {
                if (mid.top().first._type == enum_op_t::NULLTYPE) {
                    mid.pop(); return retval;
                }
                retval = performOp(mid.top().first, mid.top().second, retval);
                mid.pop();
            }
            mid.push({ var, retval }); retval = getValue();
        } 
        return 0;
    }

public:
    static auto& getInstance(std::string const& expr) {
        Calculator c(expr);
        return c;
    }
};

int main() {
    for (;;) {
        std::cout << "<calc> ";
        std::string input;
        std::getline(std::cin, input);
        Calculator::getInstance(input);
    }
    return 0;
}

Answer 1

Design

Calculator class

Looking at your Calculator class, you are doing all your work in the constructor, including printing output. The consumers of this class (the main function, in this case) never actually use the class features. This implies that your Calculator class does not actually represent an object, so it shouldn't be a class in the first place.

It seems to me that Calculator would be better as a namespace of related functions, and the functionality that is currently in the constructor could be moved to a function like void calculateFromString(std::string const& expr). This is much more intuitive than a class that only contains private member functions, and no real outward-facing functionality.

You can even accomplish the same public/private interface by moving the new Calculator namespace to a separate header file, and just putting all the helper functions in an unnamed namespace in the corresponding source file, so they are not visible to clients.

Calculator.h:

namespace Calculator {
    void calculateFromString(std::string const& expression);
}

Calculator.cpp:

namespace {
    // Helper functions placed here will not be visible outside of Calculator.cpp
    class operator_t { ... }
    auto getOperator() { ... }
    ...
}

namespace Calculator {
    void calculateFromString(std::string const& expression) {
        // Move code from Calculator constructor to here.
    }
}

main.cpp:

#include "Calculator.h"

int main() {
    while (true) {
        ...
        Calculator::calculateFromString(input);
    }
}

Code Style

• Use more descriptive variable names. The longer, the better. In general, the greater the scope of a variable, the more descriptive it should be. These are just a few examples I noticed right away:

  • p_stack - What does the p stand for? What does the stack actually do?
  • fretval - I know this is a return value, but why the f? Even if it's hungarian notation for float, evaluate returns a double, not a float, so it is misleading.
  • s, i, and a in operator_t::operator_t - The constructor parameter names should at least be named analogous to the member variables they are fulfilling. This is less egregious due to code locality, but one-character variable names are rarely a good choice.
  • Calculator::_cur - "cur" what? Current iterator? What is it iterating?

• Put public members before private in classes. You did this in operator_t, but not in Calculator. Users of a class generally care more about what they can do with a class than what the class can do with itself, so it is natural to place the public: section first.

• Group related functionality together. For example, in your Calculator class, the operator_t class definition is right in the middle of all the Calculator constructor definitions. Try to keep related functionality as vertically close as you can, so that consumers don't have to go skipping all over your codebase.

• Don't explicitly declare = delete for operator_t default constructor. If you have defined a constructor for a class, then the default constructor is automatically deleted. Explicitly deleting it is redundant. You can also safely put = delete statements in the public: section of a class, since they are technically part of the class interface - you are telling your clients "I have chosen to remove this functionality, so don't try to use it."

• Turn operator_t into a struct. Continuing off the above point, if we remove the line operator_t() = delete;, then there is nothing left in the private: section, and operator_t can be changed from a class to a struct. This also conveys our intent more precisely - operator_t is a small collection of publicly-accessible values with no inherent functionality, which is a perfect use-case for struct.

• Don't explicitly specify enum values in enum_op_t I couldn't find a case where you actually treat the enum_op_t values as characters, so there is no reason to explicitly specify each value as matching to a character. Additionally, NULLTYPE = '0' seems like an error - I suspect that what you actually wanted was the null byte '\0', not the character '0'. It should be irrelevant anyway, since the whole point of enums is that you shouldn't be concerned with their underlying values, in which case it is more conventional/"less weird" to just declare the enum constants and not assign specific values.

Answer 2

I see a number of things that may help you improve your program.

Put each statement on a separate line

Instead of crowding things together like this:

case '(': p_stack.push('('); break;

it seems to me to be more readable if each statement is given on its own line.

case '(': 
    p_stack.push('('); 
    break;

Don't use leading underscores in names

Anything in global space with a leading underscore is a reserved name in C++ (and in C). See this question for details. I tend to avoid leading underscores completely in my code.

Think of the user

There are a number of things that the program could include to make life easier for the user of this calulator. Most simply, why not provide a way to quit the program? It's as easy as adding these to lines to main

if (input == "quit") 
    break;

Also, rather than simply saying "could not parse", it might be nice to show the user exactly what token was the problem.

Never return a reference to a local!

The code currently contains this function:

static auto& getInstance(std::string const& expr) {
    Calculator c(expr);
    return c;
}

The problem is that c's scope is just the function body, so it goes out of scope (and is destroyed) when the function completes. However, we've just handed back a reference this now-destroyed variable. There are a number of ways to fix this, as in the next suggestion.

Use a free function instead of a member variable

Using a free function makes more sense to me here than using a static member function. For example, consider this function:

double calc(std::string const& expr) {
    Calculator c{expr};
    return c.fretval;
}

This also leads directly to next observation.

Separate data manipulation from output

The constructor really does two things. It calculates the result of an expression and then it outputs that to std::cout. Better would be to split that into the two logical halves. Specifically, create an expression evaluator that just returns either an answer or throws an error. Let the caller worry about how to react to that or how (and where) to print it.