7
\$\begingroup\$

I am building a Calculator in C++ OOP style. I made 3 Classes:

  • UserInterface, which is responsible for Input and Output of the User.
  • Calculation, which is responsible for the logic of the Calculator.
  • AdvancedCalculation, which inherits from Calculation and adds more operations to the Calculator.

The Calculator is functional; however I wanted to be able to write good code and not just functional code.

I was wondering if there is a way to change the Calculation Class so adding new operation types like: pow(), sqrt, log, sin, etc., is going to be easier and more convenient, mainly the switch statement in the Calculation Class.

Here is the Code for all 3 Classes:

#pragma once #include "AdvancedCalculation.h" class UserInterface { private: AdvancedCalculation C; protected: double num1; double num2; string op; double result; char input; public: void baseProgram() { using namespace std; do { cout << "Enter the first number: " << endl; cin >> num1; C.setNum1(num1); cout << "Enter an Operator: " << endl; cin >> op; C.setOp(op); cout << "Enter the second number: " << endl; cin >> num2; C.setNum2(num2); C.logic(); cout << C.getResult(); cout << "\nPress A to continue. Press any Key To Exit" << endl; cin >> input; system("cls"); } while (input == 'A' || input == 'a'); } }; #pragma once #include <vector> #include <string> #include <algorithm> #include <iostream> using namespace std; class Calculation { public: void logic() { simplifiyOp(); BaseCalculator(); } void setNum1(double num1) { this->num1 = num1; } void setNum2(double num2) { this->num2 = num2; } void setOp(string op) { transform(op.begin(), op.end(), op.begin(), ::tolower); this->op = op; } double getResult() { return result; } protected: double num1; double num2; string op; double result; vector<vector<string>> listOfOperators = { { {"+", "plus", "add"}, {"-", "minus"}, {"*", "multiply", "multiply by", "multiplied by"}, {"/", "divide"}, {"^", "power", "power to"}, {"s", "squareroot", "square root", "square root of"} } }; void Addition() { result = num1 + num2; } void Subtraction() { result = num1 - num2; } void Multiplication() { result = num1 * num2; } void Division() { result = num1 / num2; } void simplifiyOp() { for (auto operatorType : listOfOperators) { for (string wording : operatorType) { if (op == wording) { op = operatorType[0]; break; } } } } private: void BaseCalculator() { switch (op[0]) { case '+': Addition(); break; case '-': Subtraction(); break; case '*': Multiplication(); break; case '/': Division(); break; default: break; } } }; #pragma once #include "Calculation.h" class AdvancedCalculation : public Calculation { public: void logic() { simplifiyOp(); BaseCalculator(); } protected: void Powers() { result = pow(num1, num2); } void Square() { result = pow(num1, 1 / num2); } void BaseCalculator() { switch (op[0]) { case '+': Addition(); break; case '-': Subtraction(); break; case '*': Multiplication(); break; case '/': Division(); break; case '^': Powers(); break; case 's': Square(); break; default: break; } } }; 

I tried to use memcpy() so I could copy op[0] and use it in a function that could take every operator, but it didn't really work as expected. I watched some OOP tutorials, hoping there is some function that could help me get the desired result, but no use.

\$\endgroup\$
12
  • 7
    \$\begingroup\$so adding new operation types like: pow(), sqrt, log, sin, etc.. is going to be more easy and convenient -- I hate to break this to you, but you need to learn how to properly parse and tokenize the input, maybe set up an abstract syntax tree, and then use algorithms such as the shunting yard algorithm. You cannot naively write ad-hoc code like this and expect to expand on it. You will quickly see that any expansion on code like this may require you to start all over again, just to add the features in.\$\endgroup\$CommentedJan 3 at 20:00
  • 3
    \$\begingroup\$Order of operations (3 + 2 * 4, for example) and parentheses (example: pow(pow(3,2),2)) will basically illustrate the point I made in the previous comment. With the approach your current code takes, you will go crazy trying to get the new feature to work without breaking something else, and/or making the code so horrific that it becomes almost impossible to maintain the code. Using formal techniques like the shunting-yard algorithm, or some other algorithm (even using the "operator/operand stack approach with precedence level checking') is the way to write such code.\$\endgroup\$CommentedJan 3 at 20:12
  • 2
    \$\begingroup\$@Anon26 Writing a calculator and/or expression parser (usually they come as a combination) is really left up to intermediate to advanced programmers. It may look "cool" up front seeing simple two operand / single operator math working in a program ( 2+3 or 4*5, etc.), but to go beyond that is a whole different task.\$\endgroup\$CommentedJan 3 at 20:27
  • 2
    \$\begingroup\$Get out of the habit of using namespace std;\$\endgroup\$
    – Chris
    CommentedJan 4 at 0:00
  • 2
    \$\begingroup\$At the demonstrated skill level, you may want to tackle implementing a Reverse Polish Notation calculator.\$\endgroup\$
    – Chris
    CommentedJan 4 at 0:29

2 Answers 2

6
\$\begingroup\$

I don’t have time to do a full review right now; maybe I’ll come back and do one later. But I can at least help with one part of your design.

Normally, when implementing an object-oriented calculator, you start with a very abstract base class for anything in a calculation. Normally, the abstraction used is an expression.

So you start with an abstract expression base class. It only needs a single member function, which you can call result() or compute() or value()… whatever you please. It should be pure virtual, and it should return a computed value.

From that, the most basic kind of expression is just a number. You can call it value or number or whatever. Its result() function just returns the number.

Next you add more complex expressions. For example, an addition expression takes two expressions as arguments in its constructor, and its result() function calls result() on both of those, then adds the results and returns it:

class addition : public expression { std::unique_ptr<expression> _v1; std::unique_ptr<expression> _v2; public: addition(std::unique_ptr<expression> v1, std::unique_ptr<expression> v2) : _v1{std::move(v1)} , _v2{std::move(v2)} {} auto result() override -> double { return _v1->result() + _v2->result(); } }; 

Want square root? No problem:

class sqrt : public expression { std::unique_ptr<expression> _v; public: sqrt(std::unique_ptr<expression> v) : _v{std::move(v)} {} auto result() override -> double { return std::sqrt(_v->result()); } }; 

Wanna get crazy? Let’s get the incomplete elliptical integral of the third kind:

class ellint_3 : public expression { std::unique_ptr<expression> _k; std::unique_ptr<expression> _n; std::unique_ptr<expression> _f; public: sqrt(std::unique_ptr<expression> k, std::unique_ptr<expression> n, std::unique_ptr<expression> f) : _k{std::move(k)} , _n{std::move(n)} , _f{std::move(f)} {} auto result() override -> double { return std::ellint_3(_k->result(), _n->result(), _f->result()); } }; 

You can add any other operations you can imagine.

Now it’s just a matter of implementing a parser that can read (2 * sqrt(3)) / 4 and produce:

auto sqrt_expr = std::make_unique<sqrt>(std::make_unique<number>(3)); auto add_expr = std::make_unique<addition>(std::make_unique<number>(2), std::move(sqrt_expr)); auto div_expr = std::make_unique<division>(std::move(add_expr), std::make_unique<number>(4)); std::println("final result = {}", div_expr->result()); 

I’ll leave the design of the base class up to you. The hard part is really the parser. Reading (2 * sqrt(3)) / 4 and turning it into the expression tree above is not hard… but not trivial.

You might want to try first parsing reverse polish notation, and then only trying natural notation later. Parsing is easy with RPN, because you create objects as you read tokens. Like, with 1 2 +, you first create the number object for 1, then another number object for 2, then an addition object with the two previous expressions for the +. It’s much easier than 1 + 2, because when you get to the + there, you don’t know what the second argument for it is yet. Similarly, sqrt(2) is harder to parse than 2 sqrt. In the first case, you read the sqrt token, but can’t create the sqrt object yet because you don’t have its operand. In the second case, you first create the number object for 2, and then you see the sqrt, and you already have everything you need: just make a sqrt object with the number object you already have.

Anywho, that should get you started. Happy coding!

\$\endgroup\$
2
  • \$\begingroup\$Fine... Two suggestions: 1) sqrt( -1 ) is problematic..., and 2) Perhaps suggest a transformer that converts algebraic formulas to RPN then uses the same solver. Yes, one could solve by traversing a tree, but solving from a stack separates structure from operation sequence... Just a thought... Cheers! :-)\$\endgroup\$
    – Fe2O3
    CommentedJan 4 at 1:20
  • \$\begingroup\$I mean, it should go without saying that you should check for invalid input before jamming it into an expression tree and running solve(). Never mind sqrt(-1), there’s also trying to divide by zero, or put nan into a number, or a billion other things. Yes, obviously, validate your input.\$\endgroup\$
    – indi
    CommentedJan 4 at 1:30
5
\$\begingroup\$

When to use OOP

I watched some OOP tutorials, hoping there is some function that could help me get the desired result, but no use.

Object-oriented programming is a hammer, but not every software problem is a nail. You shouldn't try to force OOP on something unless you know it is the right tool for the job.

Also, OOP and classes are not the same thing. You don't need classes to write a program in an OOP style, and you can have classes without using OOP.

Let's look at your classes and OOP techniques, and see if they are a good match:

class UserInterface

This is a class with just a single function. I think the intended use case is like so:

int main() { UserInterface ui; ui.baseProgram(); } 

In that case, putting the function baseProgram() inside a class is unnecessary. You can just have a free function baseProgram(), with the member variables of UserInterface just declared inside that function, so your code would look like this:

void baseProgram() { double num1; double num2; … using namespace std; do { … } while (input == 'A' || input == 'a'); } int main() { baseProgram(); } 

Some languages require functions to be put in a class, like Java, but C++ is not one of them.

class Calculation

This class has more member functions, but again consider how it is going to be used; it will very likely always be this sequence of calls:

Calculation calculation; calculation.setNum1(num1); calculation.setNum2(num2); calculation.setOp(op); calculation.logic(); auto result = calculation.getResult(); 

That's 6 lines of code. What if you would make a single function named calculate() that takes the two numbers and the operator, and returned the result directly? Then you could rewrite all those lines to:

auto result = calculate(num1, num2, op); 

This is simpler and less error-prone: consider forgetting to call logic() or one of the set…() functions; that would cause getResult() to return nonsense without any indication of an error, whereas this is impossible with the single function call that takes all three inputs at the same time.

class AdvancedCalculation

By inheriting from Calculation you do indeed avoid some code duplication. But why do you need to have a separate Calculation and AdvancedCalculation to begin with? Why not add the advanced functionality directly to Calculation?

Another issue is that you still ended up having to duplicate all the case statements of Calculation::BaseCalculator() inside AdvancedCalculation::BaseCalculator(). Ideally, you want to avoid code duplication as much as possible, as it's only a source of potential errors.

By not making either logic() or BaseCalculatorvirtual, there is also some potentially unexpected behavior. Consider setting the numbers and an advanced operation, but then passing a reference to the base class to another function, which calls logic() and getResult() on the base class. It will then get incorrect results:

AdvancedCalculation advancedCalculation; advancedCalculation.setNum1(2); advancedCalculation.setNum2(3); advancedCalculation.setOp("^"); Calculation& calculation = advancedCalculation; calculation.logic(); auto result = calculation.getResult(); 

Again, most of the problems would go away if this wasn't a class. You can still have an advanced function call a more basic function as a fallback:

double calculate(double num1, double num2, std::string op) { … } double advancedCalculate(double num1, double num2, std::string op) { … switch (op[0]) { case '^': return std::pow(num1, num2); case 's': return std::pow(num1, 1 / num2); default: return calculate(num1, num2, op); } } 

Although again, I see no point in having two separate functions here.

Naming things

When naming variables, functions and types, be as consistent as possible. I see that some of your member functions start with a capital letter, while others start with a lower case letter.

Also avoid unnecessary abbreviations, unless they are very common abbreviations and used consistently. In the case of op, I don't think it's very common, and it's ambiguous as well (think operator, operation, option, optimize, and so on). You also use operator in some cases, so I would just consistently write it out in full everywhere. Also remember that most code editors have tab completion, so you won't save much typing by using an abbreviation anyway, it just makes reading it back later harder.

Function names should be verbs. Instead of Addition(), it should be performAddition(), or perhaps just add(). A name like logic() is very bad; it's too generic since almost all code is "logic". Maybe this should be named performCalculation() or just calculate(). Of course again, even better would be to have a free function calculate() that takes three parameters and returns the result.

Also make sure that the names are actually correct. Does Square() square a number? No; it performs a root operation, and not necessarily a square root. So root() would be better. Why is Powers() plural? That implies that it calculates multiple powers.

Avoid using system()

Don't use system("cls") to clear the screen. It's hugely inefficient; it starts a new shell process that has to interpret the command cls, which then will clear the screen, but only if you run it on Windows. On almost any other operating system, you would have to use system("clear"). Also, this assumes you don't have any environment variables or shell configuration that change the behavior of the cls command.

I recommend not clearing the screen at all, it doesn't add anything useful to your program, and in fact it will even remove the result from your calculation from the screen, when in fact that was what the user wanted to see in the first place!

If you really want to read clear the screen in C++, look at the two top voted answers of this StackOverflow question.

Prefer '\n' instead of std::endl

Instead of using std::endl, use '\n'; the former is equivalent to the latter, but also forces the output to be flushed, which is usually not necessary and can hurt performance.

Making operations extensible

For your style of program, OOP is not the right way to make it more extensible. Instead, have a look at listOfOperators: this is a data structure that can be easily added to. However, it currently only contains the list of operator names, the mapping from operator to the function that implement the operator is done in a switch-statement. But what if you could add this to listOfOperators itself? Consider this code:

double add(double num1, double num2) { return num1 + num2; } … struct Operator { std::function<double(double, double)> calculate; std::set<std::string> names; }; std::vector<Operator> operators = { {add, {"+", "plus", "add"}}, {minus, {"-", "minus"}}, … }; double calculate(double num1, double num2, std::string operatorName) { for (auto operator: operators) { if (operator.names.contains(operatorName)) { return operator.calculate(num1, num2); } } throw std::runtime_error("Invalid operator"); } 

Now adding a new operator the the list is as easy as defining the function that does the calculation and adding a single line to the declaration of operators. You could even use lambda functions:

std::vector<Operator> operators = { {[](auto num1, auto num2){ return num1 + num2; }, {"+", "plus", "add"}}, {[](auto num1, auto num2){ return num1 - num2; }, {"-", "minus"}}, … }; 
\$\endgroup\$
2
  • \$\begingroup\$Thank you for the feedback, the reason I chose OOP for this program is because I started C++ 2 weeks ago, coming from C after using it for a Month. As you know C doesn't support OOP. So my friend who suggested rebuilding my projects in OOP, to get used to working across multiple files, and getting out of C mentality. I directly implemented of your suggestions to the program. I am currently rewriting a To Do List in this style again. Would you recommend doing it differently?\$\endgroup\$
    – Anon26
    CommentedJan 5 at 11:47
  • 1
    \$\begingroup\$C++ has a lot more to offer than just OOP. What the best techniques to use are depends on the problem though. You can post your rewritten To Do List as a new question here on Code Review and we can discuss it there.\$\endgroup\$CommentedJan 5 at 13:32

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.