4
\$\begingroup\$

I've written a simple calculator to train my C++ skills. Beware as this is my 1st project in C++ and it may contain some bad practices or something of the likes.

I want to know if my code follows good practices, is readable, moreless bug free and manages memory properly (I come from a Java background so that's been hard and sometimes confusing for me).

It supports addition, subtraction, multiplication and division.

Input can be:

  • a single number (e.g. 2 returns 2)
  • a single expression (e.g. 2+2 returns 4)
  • a complex expression (e.g. 2+2+2 returns 6, 2+2*2/8 returns 1, 2+(2*2) / 6 * 3 + 2 returns 20)

I did not yet implement operator precedence and aren't even sure how to do it yet, but 2 + 2 * 2 = 8 (unless you write it as 2 + (2 * 2) - then it equals 6).

main.cpp (main logic is here)

#include <cstdio>
#include <algorithm>
#include <memory>
#include <unordered_map>
#include <map>
#include <iostream>

#include "expressions/ConstExpression.h"
#include "expressions/CompoundExpression.h"
#include "operations/MultiplicationOperation.h"
#include "operations/AdditionOperation.h"
#include "operations/SubtractionOperation.h"
#include "operations/DivisionOperation.h"

// single as in not interpreting nested parentheses
std::string interpret_single_paren(std::string::const_iterator &it) {
    using std::size_t;
    using std::string;

    int level = 0;
    string paren{};
    while (true) {
        if (*it == '(') {
            if (level != 0)
                paren += *it;
            level += 1;
        } else if (*it == ')') {
            level -= 1;
            if (level == 0)
                break;
            else
                paren += *it;
        } else {
            paren += *it;
        }
        it++;
    }

    return paren;
}


std::shared_ptr<Expression> parse_number(std::string::const_iterator &it) {
    std::string sNumber{};
    while (std::isdigit(*it) || *it == '.') {
        sNumber.push_back(*(it++));
    }
    it--;

    float number = std::stof(sNumber);
    return std::make_shared<ConstExpression>(number);
}

static std::map<char, Operation *> charToOperationMap{
        {'+', &AdditionOperation::getInstance()},
        {'-', &SubtractionOperation::getInstance()},
        {'*', &MultiplicationOperation::getInstance()},
        {'/', &DivisionOperation::getInstance()},
};


std::shared_ptr<Expression> parse_expr(const std::string &string) {
    std::vector<std::shared_ptr<Expression>> expressions{};
    std::shared_ptr<Expression> topExpression{nullptr};
    Operation *cOperation = nullptr;

    for (auto it = string.cbegin(); it != string.cend(); ++it) {
        if (*it == ' ') continue;
        else if (std::isdigit(*it) || *it == '.' /*as in .5*/) {
            std::shared_ptr<Expression> expr = parse_number(it);
            expressions.push_back(expr);
            continue;
        } else if (*it == '(') {
            std::string paren = interpret_single_paren(it);
            std::shared_ptr<Expression> expr = parse_expr(paren);
            expressions.push_back(expr);
            continue;
        }

        {
            auto operation = charToOperationMap.find(*it);
            if (operation != charToOperationMap.end()) {
                if (cOperation != nullptr) {

                    // Optimization to allow for 2+2+2 to be created as CompoundExpr(2,2,2)
                    // instead of CompoundExpr(CompoundExpr(2,2),2)
                    if (cOperation != operation->second) {
                        topExpression = std::make_shared<CompoundExpression>(expressions, *cOperation);
                        expressions.clear();
                        expressions.push_back(topExpression);
                    }
                }
                cOperation = operation->second;
                continue;
            }
        }
    }

    if (cOperation != nullptr)
        topExpression = std::make_shared<CompoundExpression>(expressions, *cOperation);
    else if (expressions.size() == 1)
        topExpression = std::shared_ptr<Expression>{expressions[0]};
    else
        throw std::invalid_argument(
                "Provided string is invalid (e.g.: 2 2 or -7 20, i.e. there's more than 1 number and no operators)");

    return topExpression;
}

int main() {
    while (true) {
        std::cout << "\nInput expression: ";
        std::string line;
        std::getline(std::cin, line);
        if (line == "exit") break;
        std::shared_ptr<Expression> expr = parse_expr(line);
        std::cout << "Result of " << *expr << ": " << expr.get()->evaluate() << std::endl;
    }

    std::cout << "Bye" << std::endl;
    return 0;
}

Expression.h

#ifndef CALCULATOR_EXPRESSION_H
#define CALCULATOR_EXPRESSION_H

#include <ostream>

class Expression {
protected:
    virtual std::ostream &output(std::ostream &os) const = 0;

public:
    virtual float evaluate() const = 0;

    friend std::ostream &operator<<(std::ostream &os, const Expression &expression) {
        return expression.output(os);
    }
};

#endif //CALCULATOR_EXPRESSION_H

ConstExpression.h

#ifndef CALCULATOR_CONSTEXPRESSION_H
#define CALCULATOR_CONSTEXPRESSION_H


#include <ostream>
#include "Expression.h"

class ConstExpression : public Expression {
    const float value;

protected:
    std::ostream &output(std::ostream &os) const override {
        return os << value;
    }

public:
    explicit ConstExpression(float value) : value(value) {}

    inline float evaluate() const override {
        return value;
    }
};

#endif //CALCULATOR_CONSTEXPRESSION_H

CompoundExpression.h

#ifndef CALCULATOR_COMPOUNDEXPRESSION_H
#define CALCULATOR_COMPOUNDEXPRESSION_H

#include <utility>
#include <vector>
#include <iterator>
#include <ostream>
#include "Expression.h"
#include "../operations/Operation.h"

class CompoundExpression : public Expression {
private:
    const std::vector<std::shared_ptr<Expression>> exprs;
    const Operation &operation;
protected:
    std::ostream &output(std::ostream &os) const override {
        for (size_t i = 0; i < exprs.size() - 1; ++i) {
            os << *exprs[i] << " " << operation << " ";
        }
        os << *exprs[exprs.size() - 1];

        return os;
    }

public:
    explicit CompoundExpression(std::vector<std::shared_ptr<Expression>> exprs, const Operation &operation) :
            exprs(exprs), // exprs(std::vector<std::shared_ptr<Expression>>{exprs}),
            operation(operation) {}

    float evaluate() const override {
        if (exprs.empty()) return 0;
        float acc = exprs[0]->evaluate();

        for (size_t i = 1; i < exprs.size(); ++i) {
            acc = operation.evaluate(acc, exprs[i]->evaluate());
        }

        return acc;
    }
};

#endif //CALCULATOR_COMPOUNDEXPRESSION_H

Operation.h

#ifndef CALCULATOR_OPERATION_H
#define CALCULATOR_OPERATION_H


#include <vector>
#include <ostream>

class Operation {
private:
    static std::vector<Operation *> _operations;

public:
    Operation() {
        _operations.push_back(this);
    }

    virtual const std::string name() const = 0;

    virtual float evaluate(float a, float b) const = 0;

    friend std::ostream &operator<<(std::ostream &os, const Operation &operation);

    static const std::vector<Operation *> operations() {
        return _operations;
    };
};

#endif //CALCULATOR_OPERATION_H

Operation.cpp

#include "Operation.h"

std::vector<Operation*> Operation::_operations{};

std::ostream &operator<<(std::ostream &os, const Operation &operation) {
    return os << operation.name();
}

AdditionOperation.h

#ifndef CALCULATOR_ADDITIONOPERATION_H
#define CALCULATOR_ADDITIONOPERATION_H


#include <ostream>
#include "Operation.h"

class AdditionOperation : public Operation {
private:
    AdditionOperation() = default;

    const std::string _name{"plus"};

public:
    static AdditionOperation &getInstance() {
        static AdditionOperation operation;

        return operation;
    }

    AdditionOperation(AdditionOperation const &) = delete;

    void operator=(AdditionOperation const &)  = delete;

    const std::string name() const override {
        return _name;
    }

    float evaluate(float a, float b) const override {
        return a + b;
    }
};


#endif //CALCULATOR_ADDITIONOPERATION_H

SubtractionOperation.h

#ifndef CALCULATOR_SUBTRACTIONOPERATION_H
#define CALCULATOR_SUBTRACTIONOPERATION_H


#include <ostream>
#include "Operation.h"

class SubtractionOperation : public Operation {
private:
    SubtractionOperation() = default;

    const std::string _name{"minus"};

public:
    static SubtractionOperation &getInstance() {
        static SubtractionOperation operation;

        return operation;
    }

    SubtractionOperation(SubtractionOperation const &) = delete;

    void operator=(SubtractionOperation const &)  = delete;

    const std::string name() const override {
        return _name;
    }

    float evaluate(float a, float b) const override {
        return a - b;
    }
};


#endif //CALCULATOR_SUBTRACTIONOPERATION_H

MultiplicationOperation.h

#ifndef CALCULATOR_MULTIPLICATIONOPERATION_H
#define CALCULATOR_MULTIPLICATIONOPERATION_H


#include <ostream>
#include "Operation.h"

class MultiplicationOperation : public Operation {
private:
    MultiplicationOperation() = default;
    const std::string _name{"times"};

public:
    static MultiplicationOperation &getInstance() {
        static MultiplicationOperation operation;

        return operation;
    }

    MultiplicationOperation(MultiplicationOperation const &) = delete;

    void operator=(MultiplicationOperation const &)  = delete;

    const std::string name() const override {
        return _name;
    }

    float evaluate(float a, float b) const override {
        return a * b;
    }
};


#endif //CALCULATOR_MULTIPLICATIONOPERATION_H

DivisionOperation.h

#ifndef CALCULATOR_DIVISIONOPERATION_H
#define CALCULATOR_DIVISIONOPERATION_H


#include <ostream>
#include "Operation.h"

class DivisionOperation : public Operation {
private:
    DivisionOperation() = default;

    const std::string _name{"divided by"};

public:
    static DivisionOperation &getInstance() {
        static DivisionOperation operation;

        return operation;
    }

    DivisionOperation(DivisionOperation const &) = delete;

    void operator=(DivisionOperation const &)  = delete;

    const std::string name() const override {
        return _name;
    }

    float evaluate(float a, float b) const override {
        return a / b;
    }
};


#endif //CALCULATOR_DIVISIONOPERATION_H
\$\endgroup\$
  • \$\begingroup\$ @Incomputable I agree I've kind of got a lot of ifs in the interpret_single_paren function but other than that I can't see where else are there too many of them \$\endgroup\$ – Mibac Dec 16 '17 at 22:05
  • \$\begingroup\$ I meant all of those classes and inheritance. \$\endgroup\$ – Incomputable Dec 16 '17 at 22:39
3
\$\begingroup\$

Overall, I find your code to be great. There are just a few nitpicks here and there that I would like to mention:

  1. You should order your includes more, and also differently. More in a sense of alphabetically and differently in a sense of grouping. Why should you order your includes alphabetically? Simply to increase readability and to facilitate include checking. However, you should also group your includes by what project they belong and order those groups. Generally, I prefer: The header your current file is implementing (if any), then any headers from the same project, then headers from different projects and libraries and finally standard headers. This order has the benefit of providing an additional error check: If one of your headers is missing an include, including that header first will make the compiler tell you about it. Otherwise, you might just happen to include the missing header first because you need some functionality from it and hide the missing include.
  2. You should consider passing iterators by value. Usually, iterators are designed to be lightweight and easily copyable, which is why the standard library always takes them by value, too. You are not going to gain much by passing them by reference, so you should prefer the clearer and more idiomatic pass by value.
  3. I find your use of std::shared_ptr excessive. For example, in my opinion parse_number should rather return a std::unique_ptr because you're transferring ownership here. If you are looking for rules on when to use which smart pointer (or even a raw pointer), you should take a look at the Cpp Core Guidelines.
  4. Please don't use std::endl. It doesn't only do what its name suggests, but also flushes the output buffer which is almost never necessary and can be harmful to performance. If you really need to flush the buffer, you should use flush() instead.
  5. You actually have a missing include in CompoundExpression.h. You are using size_t (which really should be std::size_t as the global name is not guaranteed), but are missing an include for cstddef (or one of the other headers that define it).
\$\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.