String Calculator 2

Question

I made a calculator a few days ago and posted the code here. I got some very helpful feedback. So I remade the calculator and learned a lot in the process. Here's my new code:

In the header file:

#include <string>

template <typename NUMTYPE>
std::string calculate(const std::string&);
template <typename NUMTYPE>
std::string calculateRPN(const std::string&);

std::string toRPN(const std::string&);

In the source file:

#include <iostream>
#include <iomanip>
#include <sstream>
#include <string>
#include <vector>
#include <stdexcept>
#include <cmath>

// forward declarations:

template std::string calculate<long double>(const std::string&);
template std::string calculateRPN<long double>(const std::string&);
template std::string calculate<long long>(const std::string&);
template std::string calculateRPN<long long>(const std::string&);
template std::string calculate<double>(const std::string&);
template std::string calculateRPN<double>(const std::string&);
template std::string calculate<long>(const std::string&);
template std::string calculateRPN<long>(const std::string&);
template std::string calculate<int>(const std::string&);
template std::string calculateRPN<int>(const std::string&);
template std::string calculate<float>(const std::string&);
template std::string calculateRPN<float>(const std::string&);
template std::string calculate<short>(const std::string&);
template std::string calculateRPN<short>(const std::string&);


template <typename NUMTYPE>
std::string calculate(const std::string &expression)
{
    if (expression.empty())
        return "";
    return calculateRPN<NUMTYPE>(toRPN(expression));
}

inline bool isNumber(const char);
inline bool isLetter(const char);
inline bool isOperator(const char);
template <typename NUMTYPE>
NUMTYPE applyOperator(const char operation, NUMTYPE&, NUMTYPE&);
template <typename NUMTYPE>
void applyFunction(std::string &function, NUMTYPE &argument);
template <typename NUMTYPE>
NUMTYPE factorial(NUMTYPE);

// pi and e aren't defined in the c++ standard until c++20 so I define them here
static const long double pi_num = 3.1415926535897932;
static const long double e_num = 2.7182818284590452;

template <typename NUMTYPE>
std::string calculateRPN(const std::string &expression)
{
    // https://en.wikipedia.org/wiki/Reverse_Polish_notation#Postfix_evaluation_algorithm
    if (expression.empty())
        return "";
    std::vector<NUMTYPE> number_stack;
    std::stringstream in(expression);
    std::string word;
    NUMTYPE num1, num2;

    try
    {
        while (in >> word)
        {
            if (word == "(" || word == ")")
                throw std::runtime_error("Syntax Error");
            else if (isNumber(word.front()))
            {
                std::stringstream numstream(word);
                numstream >> num1;
            }
            else if (isOperator(word.front()))
            {
                if (word.size() > 1) // negative number
                {
                    if (word.front() != '-' || !isNumber(word[1]))
                        throw std::runtime_error("Operators must be space-seperated");
                    std::stringstream numstream(word);
                    numstream >> num1;
                    number_stack.push_back(num1);
                    continue;
                }
                if (number_stack.empty())
                    throw std::runtime_error("Too Many Operators");
                num1 = number_stack.back();
                number_stack.pop_back();
                num2 = number_stack.back();
                number_stack.pop_back();
                num1 = applyOperator(word.front(), num1, num2);
            }
            else if (isLetter(word.front()))
            {
                // dealing with mathematical constants
                if (word == "pi")
                {
                    number_stack.push_back(pi_num);
                    continue;
                }
                if (word == "e")
                {
                    number_stack.push_back(e_num);
                    continue;
                }

                // dealing with functions
                num1 = number_stack.back();
                number_stack.pop_back();
                if (word == "min")
                {
                    num2 = number_stack.back();
                    number_stack.pop_back();
                    num1 = num1 < num2 ? num1 : num2;
                }
                else if (word == "max")
                {
                    num2 = number_stack.back();
                    number_stack.pop_back();
                    num1 = num1 > num2 ? num1 : num2;
                }
                else
                    applyFunction(word, num1);
            }
            else
                throw std::runtime_error("Unknown Symbol");

            number_stack.push_back(num1);
        }

        if (number_stack.size() > 1)
            throw std::runtime_error("Too Many Numbers");

        std::stringstream answer;
        answer << std::setprecision(16) << number_stack.back();
        answer >> word;
        return word;
    }
    catch(const std::exception& error)
    {
        return error.what();
    }
}

inline bool isNumber(const char character)
{
    if ((character >= '0' && character <= '9') || character == '.')
        return true;
    else
        return false;
}

inline bool isLetter(const char character)
{
    if ((character >= 'a' && character <= 'z') || (character >= 'A' && character <= 'Z'))
        return true;
    else
        return false;
}

inline bool isOperator(const char character)
{
    if (character == '+' || character == '-' || character == '*' || character == '/' ||
        character == '%' || character == '^' || character == '(' || character == ')')
        return true;
    else
        return false;
}

template <typename NUMTYPE>
NUMTYPE applyOperator(const char operation, NUMTYPE& num1, NUMTYPE& num2)
{
    if (operation == '+')
        return num1 + num2;
    else if (operation == '-')
        return num1 - num2;
    else if (operation == '*')
        return num1 * num2;
    else if (operation == '/')
    {
        if (num1 == 0)
            throw std::runtime_error("Math Error");
        return num2 / num1;
    }
    else if (operation == '%')
    {
        if (num1 == 0)
            throw std::runtime_error("Math Error");
        return (long long) num2 % (long long) num1;
    }
    else if (operation == '^')
    {
        if (num1 == 0 && num2 == 0)
            throw std::runtime_error("Math Error");
        return pow(num2, num1);
    }
    else
        throw std::runtime_error("Unknown Symbol");
}

template <typename NUMTYPE>
void applyFunction(std::string &function, NUMTYPE &argument)
{
    if (function == "abs")
        argument = fabs(argument);

    else if (function == "sqrt")
        argument = sqrt(argument);
    else if (function == "cbrt")
        argument = cbrt(argument);

    else if (function == "sin")
        argument = sin(argument);
    else if (function == "cos")
        argument = cos(argument);
    else if (function == "tan")
        argument = tan(argument);
    else if (function == "cot")
        argument = 1 / tan(argument);
    else if (function == "sec")
        argument = 1 / cos(argument);
    else if (function == "csc")
        argument = 1 / sin(argument);
    else if (function == "arctan")
        argument = atan(argument);
    else if (function == "arcsin")
        argument = asin(argument);
    else if (function == "arccos")
        argument = acos(argument);
    else if (function == "arccot")
        argument = atan(1 / argument);
    else if (function == "arcsec")
        argument = acos(1 / argument);
    else if (function == "arccsc")
        argument = asin(1 / argument);

    else if (function == "sinh")
        argument = sinh(argument);
    else if (function == "cosh")
        argument = cosh(argument);
    else if (function == "tanh")
        argument = tanh(argument);
    else if (function == "coth")
        argument = 1 / tanh(argument);
    else if (function == "sech")
        argument = 1 / cosh(argument);
    else if (function == "csch")
        argument = 1 / sinh(argument);
    else if (function == "arctanh")
        argument = atanh(argument);
    else if (function == "arcsinh")
        argument = asinh(argument);
    else if (function == "arccosh")
        argument = acosh(argument);
    else if (function == "arccoth")
        argument = atanh(1 / argument);
    else if (function == "arcsech")
        argument = acosh(1 / argument);
    else if (function == "arccsch")
        argument = asinh(1 / argument);

    else if (function == "log")
        argument = log10(argument);
    else if (function == "ln")
        argument = log(argument);
    else if (function == "exp")
        argument = exp(argument);

    else if (function == "gamma")
        argument = tgamma(argument);
    else if (function == "erf")
        argument = erf(argument);

    else
        throw std::runtime_error("Unknown Function");
}

template <typename NUMTYPE>
NUMTYPE factorial(NUMTYPE number)
{
    if (number < 0)
        throw std::runtime_error("Math Error");

    NUMTYPE res = 1;
    while (number > 1)
    {
        res *= number;
        --number;
    }
    return res;
}

// functions for "toRPN"

inline char precedence(const char operation);
void parseNumber(const std::string &in, std::string &out, std::string::size_type &index);
void parseFunction(const std::string &in, std::string::size_type &index, std::vector<std::string> &operation_stack);
void parseOperator(const char operation, std::string &out, std::vector<std::string> &operation_stack);
void pushOut(std::vector<std::string> &operation_stack, std::string &out);
bool pushOut_cond(const char operation, std::vector<std::string> &operation_stack);
bool pi_cond(const std::string &expression, std::string::size_type &index);
bool e_cond(const std::string &expression, std::string::size_type &index);

// converts a mathematical expression into Reverse Polish Notation using shunting-yard algorithm
std::string toRPN(const std::string &expression)
{
    // https://en.m.wikipedia.org/wiki/Shunting-yard_algorithm

    std::string expression_RPN;
    expression_RPN.reserve(expression.length());
    std::vector<std::string> operation_stack;
    // for dealing with omitted multiplication signs like "2sin(x)cos(x)" or "5(4+3(2+1))"
    bool number_flag = false, rightParen_flag = false;
    try
    {
        for (std::string::size_type i = 0; i < expression.length(); ++i)
        {
            if (isNumber(expression[i]))
            {
                parseNumber(expression, expression_RPN, i);
                if (rightParen_flag) // omitted multiplication sign
                    parseOperator('*', expression_RPN, operation_stack);
                if (number_flag) // consecutive numbers
                    throw std::runtime_error("Syntax Error");

                number_flag = true;
                rightParen_flag = false;
            }
            else if (isLetter(expression[i]))
            {
                if (number_flag || rightParen_flag) // omitted multiplication sign
                    parseOperator('*', expression_RPN, operation_stack);

                // dealing with mathematical constants
                if (pi_cond(expression, i))
                {
                    expression_RPN.append("pi ");
                    // treat as righ parenthesis (for omitted multiplication signs)
                    number_flag = false;
                    rightParen_flag = true;
                    ++i;
                    continue;
                }
                if (e_cond(expression, i))
                {
                    expression_RPN.append("e ");
                    // treat as righ parenthesis (for omitted multiplication signs)
                    number_flag = false;
                    rightParen_flag = true;
                    continue;
                }

                // dealing with functions
                parseFunction(expression, i, operation_stack);
                number_flag = false;
                rightParen_flag = false;
            }
            else if (isOperator(expression[i]))
            {
                // consecutive operators
                if (!number_flag && !rightParen_flag && expression[i] != '(')
                {
                    if (expression[i] == '-') // negative sign (instead of minus operation)
                    {
                        expression_RPN.append("-1 ");
                        parseOperator('*', expression_RPN, operation_stack);
                        continue;
                    }
                    else
                        throw std::runtime_error("Syntax Error");
                }

                if ((number_flag || rightParen_flag) && expression[i] == '(') // omitted multiplication sign
                    parseOperator('*', expression_RPN, operation_stack);
                parseOperator(expression[i], expression_RPN, operation_stack);

                if (expression[i] == ')')
                    rightParen_flag = true;
                else
                    rightParen_flag = false;
                number_flag = false;
            }
            else if (expression[i] == '!')
            {
                if (number_flag || rightParen_flag)
                    expression_RPN.append("! ");
                else
                    throw std::runtime_error("Syntax Error");

                // treat as righ parenthesis (for omitted multiplication signs)
                number_flag = false;
                rightParen_flag = true;
            }
            else if (expression[i] == ',')
            {
                number_flag = false;
                rightParen_flag = false;
            }
            else if (expression[i] == ' ')
                continue;
            else
                throw std::runtime_error("Unknown Symbol");
        }
        while (!operation_stack.empty())
        {
            if (operation_stack.back() == "(")
                throw std::runtime_error("Mismatched Parentheses");
            pushOut(operation_stack, expression_RPN);
        }
    }
    catch (const std::exception &error)
    {
        std::cerr << error.what() << '\n';
        return "";
    }

    expression_RPN.pop_back(); // removing the extra space
    return expression_RPN;
}

inline char precedence(const char operation)
{
    if (operation == '+' || operation == '-')
        return 0;
    else if (operation == '*' || operation == '/' || operation == '%')
        return 1;
    else if (operation == '^')
        return 2;
    else
        return 3;
}

void parseNumber(const std::string &in, std::string &out, std::string::size_type &index)
{
    out.push_back(in[index]);
    while (index + 1 < in.length() && isNumber(in[index + 1]))
    {
        ++index;
        out.push_back(in[index]);
    }
    // handling decimals (only allowing one decimal point per number)
    if (index + 1 < in.length() && in[index + 1] == '.')
    {
        while (index + 1 < in.length() && isNumber(in[index + 1]))
        {
            ++index;
            out.push_back(in[index]);
        }
    }
    out.push_back(' ');
}

void parseFunction(const std::string &in, std::string::size_type &index, std::vector<std::string> &operation_stack)
{
    std::string buffer;
    buffer.push_back(in[index]);
    while (index + 1 < in.length() && isLetter(in[index + 1]))
    {
        ++index;
        buffer.push_back(in[index]);
    }
    operation_stack.push_back(buffer);
}

void parseOperator(const char operation, std::string &out, std::vector<std::string> &operation_stack)
{
    if (operation == '(')
    {
        operation_stack.push_back(std::string(1, operation));
        return;
    }
    if (operation == ')')
    {
        while (!operation_stack.empty() && operation_stack.back() != "(")
            pushOut(operation_stack, out);
        if (operation_stack.empty()) // no left paranthesis '(' found
            throw std::runtime_error("Mismatched Parentheses");
        else // left paranthesis '(' found
            operation_stack.pop_back();
        return;
    }

    while (pushOut_cond(operation, operation_stack))
        pushOut(operation_stack, out);
    operation_stack.push_back(std::string(1, operation));
}

void pushOut(std::vector<std::string> &operation_stack, std::string &out)
{
    out.append(operation_stack.back());
    out.push_back(' ');
    operation_stack.pop_back();
}

bool pushOut_cond(const char operation, std::vector<std::string> &operation_stack)
{
    if (!operation_stack.empty() && operation_stack.back() != "(")
    {
        // a function is at the top of the stack
        if (isLetter(operation_stack.back().front()))
            return true;
        // an operation with greater precedence is at the top of the stack
        else if (precedence(operation_stack.back().front()) > precedence(operation))
            return true;
        else if (precedence(operation_stack.back().front()) == precedence(operation))
        {
            // the operation isn't right-to-left associative
            if (operation != '^')
                return true;
            else
                return false;
        }
        else
            return false;
    }
    else
        return false;
}

bool pi_cond(const std::string &expression, std::string::size_type &index)
{
    if (expression.substr(index, 2) == "pi")
    {
        if (index + 2 == expression.size() || !isLetter(expression[index + 2]))
            return true;
        else
            return false;
    }
    else
        return false;
}

bool e_cond(const std::string &expression, std::string::size_type &index)
{
    if (expression[index] == 'e')
    {
        if (index + 1 == expression.size() || !isLetter(expression[index + 1]))
            return true;
        else
            return false;
    }
    else
        return false;
}

Possible use:

int main()
{
    std::string expression;
    std::getline(std::cin, expression);
    while (expression != "end")
    {
        std::cout << "= " << calculate<long double>(expression) << "\n\n";
        std::getline(std::cin, expression);
    }
}

Two questions:

• I'm fairly new to exception handling; Did do it correctly in my code?
• There were some functions that weren't part of the "calculator". So I didn't define them in the header file. Is this bad practice or good practice?

Suggestions and ideas are always welcome :)

Answer 1

Answers to Specific Questions

There were some functions that weren't part of the "calculator". So I didn't define them in the header file. Is this bad practice or good practice?

It is sometimes necessary to keep some things private, these functions don't need a prototype in the header file.

I'm fairly new to exception handling; Did do it correctly in my code?

The problem with your exception handling is that you are returning the error,what() value as a successful word in at least one case. How does the outer program know that this is an error and stops the processing. It is possible that they try{} catch{} implementation is at a too low level and may need to be at a higher level in the programming so that the program resets and doesn't try to process the string.

General Observations

One thing to always keep in mind when designing and writing software is maintainability. Functionality always grows as a program matures and this means that there are changes that must be made. You may win the lottery or inherit a lot of money so you may not be the one maintaining the code. This code will not be easy to modify by other people.

It also seems that you may have missed some of the suggestions that EmilyL made (make sure your code is working properly).

Separate Functionality

There are 2 sets of functionality here, parsing the expressions and performing the calculations, split the functionality so that first the input is entirely parsed, and then calculate the value if the expression is legal, don't try to do everything at once. This will simplify writing and debugging the code. Build a parse tree of operators and operands and then process the parse tree separately. There is really no need to convert to Reverse Polish Notation internally.

Quite possibly there should be 2 classes used by the program, a parser and then a calculator. The parsing algorithm and the calculating algorithm should be in separate source files with separate header files.

Complexity

The function std::string toRPN(const std::string &expression) is far too complex (does too much in one function) and should have blocks of code broken out into more functions. In the dinosaur age of computing, functions that were more than one page long (about 60 lines) were considered too long to be understandable. In the modern age, any function that does not fit on a screen is too long.

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

Let the Compiler do the Optimization

In the C++ language the keyword inline is obsolete. Optimizing compilers do a much better job of deciding what functions should be inlined. There isn't any other use for the keyword inline other than optimization.

Prefer Data Structures Over Long If Then Else If Statements

In the function void applyFunction(std::string &function, NUMTYPE &argument) the very long if then else if statement might be better implemented using std::map. This would make adding or deleting operations much easier because the map is easier to add to. It would also greatly reduce the amount of code in this function.

Use the Conditional (or Ternary) Operator

The functions inline bool isNumber(const char character) inline bool isLetter(const char character) and inline bool isOperator(const char character) could all be much shorter, in the case of isNumber() and isLeter() they are one liners if you use the ternary operator.

inline bool isNumber(const char character)
{
    return ((character >= '0' && character <= '9') || character == '.');
}

inline bool isLetter(const char character)
{
    return ((character >= 'a' && character <= 'z') || (character >= 'A' && character <= 'Z'));
}

If you include <cctype> this becomes even simpler, isLetter() can simply be replaced by isalpha() and isNumber() can be simplified with

inline bool isNumber(const char character)
{
    return (isdigit(character) || character == '.');
}

The function inline bool isOperator(const char character) is easier to maintain if it written in the following manner.

bool isOperator(const char character)
{
    std::vector<char>operators = {'+', '-', '*', '/', '&', '^', '(', ')'};
    for (char m_operator: operators)
    {
        if (m_operator == character)
        {
            return true;
        }
        return false;
    }
}

Answer 2

I am not a pro coder but to my eye your try/catch block seems very correct. You did covered all types of error that may occur during the execution of the code if given an incorrect input. I would say that the exception handling is implemented correctly.
For the second question, most coders would agree that it is a good practice to drop everything that is not required in the code and removing stuff that would not effect the functionality a tiny bit. These pieces of code may increase the execution and compile time which is a judging factor in many coding competitions. In your case, removing the header files that are not required in the code is a good practice.