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 BaseCalculator
virtual
, 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 return
s 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"}}, … };
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\$2+3
or4*5
, etc.), but to go beyond that is a whole different task.\$\endgroup\$using namespace std;
\$\endgroup\$