Simple calculator with classes in C++

Question

I came to C++ from Java. This is my attempt at making a calculator on about 2 days worth of knowledge. I don't really know what is redundant or unnecessary. Nor do I know of the better ways to achieve this effect.

First class (it does the math):

#pragma once
#include <iostream>
#include <sstream>
#include <string>
using namespace std;
class a {
public:
double add(double a, double b) {
 return a + b;
}
double sub(double a, double b) {
 return a - b;
}
double mult(double a, double b) {
 return a * b;
}
double div(double a, double b) {
 return a / b;
}
};

Second class (it makes the strings):

#pragma once
#include <iostream>
#include <sstream>
#include <string>
using namespace std;
class text {
public:
string add(double a, double b, double ans) {
 return to_string(a) + " + " + to_string(b) + " = " + to_string(ans);
}
string sub(double a, double b, double ans) {
 return to_string(a) + " - " + to_string(b) + " = " + to_string(ans);
}
string div(double a, double b, double ans) {
 return to_string(a) + " / " + to_string(b) + " = " + to_string(ans);
}
string mult(double a, double b, double ans) {
 return to_string(a) + " * " + to_string(b) + " = " + to_string(ans);
}
string first() {
 return "First Number: ";
}
string second() {
 return "Second Number: ";
}
string op() {
 return "Operator: ";
}
string wrong() {
 return "Invalid Operator!";
}
};

Main method:

#include "a.h"
#include "text.h"
#include <iostream>
#include <sstream>
#include <string>
using namespace std;
int main() {
text ask;
a mather;
string temp;
char op;
double a, b, ans;
cout << ask.op();
cin.get(op);
cout << ask.first();
cin >> a;
cout << ask.second();
cin >> b;
switch (op) {
case '+':
 ans = mather.add(a, b);
 temp = ask.add(a, b, ans);
 break;
case '-':
 ans = mather.sub(a, b);
 temp = ask.sub(a, b, ans);
 break;
case '/':
 ans = mather.div(a, b);
 temp = ask.div(a, b, ans);
 break;
case '*':
 ans = mather.mult(a, b);
 temp = ask.mult(a, b, ans);
 break;
default: 
 cout << "Invalid Operator!";
}
cout << temp;
cin.get();
cin.get(); //I don't know how to avoid using this twice
return 0;
}

Answer 1

Welcome to C++.

A few notes:

• When working outside of a toy project, avoid using namespace std. For why, see here.
• In addition, when working outside of a toy project, I encourage you to define your classes within the scope of a namespace. Only you can fight namespace pollution.
• Your a header does not make use of the iostream, string, or stringstream headers. They need not be included in that header.
• Your text header does not make use of the iostream or stringstream headers. They need not be included in that header.
• Header includes are transitive. Because your main function includes your text header which in turn includes the string header, the main.cpp need not also include string.
• Your main implementation file does not make use of the sstream headers. They need not be included in that file.
• Your a class methods made no use of the class instance. That said, these methods could be declared as static.
• Your text class methods made no use of the class instance. That said, these methods could also be declared as static.
• When you write to the terminal, you probably want a next line character. You can do this either by appending a \n to the output or by calling to std::endl in the iostream header. For more on the distinction, please see this post.
• return 0; is actually unnecessary to indicate successful execution of the main function as of C99. reference

Also, please do not return a constexpr char *. Assuming you haven't specified otherwise, your compiler is very likely compiling to the C++ 11 specification. In which case, your string methods will be subject to return value optimization outside of debug mode and no temporary variables or heap allocations will be made.

• ChrisWue: Nice job, except: don't build on transitive header includes - that way madness lies. Every file should include exactly all the headers it requires, no more and no less.
• HolyBlackCat: I would also add that classes a and text should be namespaces.

Answer 2

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

Use consistent formatting

Using consistent formatting helps readers of your code understand it without distraction. This code is not badly formatted, but it could be more consistent.

Use better naming

The class named a is not well named. A better name might be Operators.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. Know when to use it and when not to (as when writing include headers).

Don't write Java in C++

C++ and Java each have their own idioms and practices. In particular, the text class (which is not a very good name in either language!) has functions which return static strings. That might make sense in Java, but it's very odd in C++, not least because unlike in Java, in C++ we care about object creation and destruction. For example, the code contains this:

cout << ask.first();

And the function is this:

string first() {
 return "First Number: ";
}

This is very un-C++-like because it creates a std::string every time the function is invoked and detroys it when the line is complete. Better would be to either create a const std::string or better a constexpr char * which needs no constructor or destructor.

Omit return 0

When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no reason to put return 0; explicitly at the end of main.

• WorldSEnder: Alternative for the text class: use references const std::string&. That of course, should be declared as a static constant of the text class.
• Daniel Jour: I'd strongly disagree with the "omit return 0;" part. While I don't want to start a debate about this (as they're usually subjective and not very helpful) I think to have read somewhere that it has been considered to make the final return necessary by some future standard. I cannot find the source atm though.
• Edward: @DanielJour: Someone usually disagrees with that, but it has been an explicit part of the C++ language for over 20 years now, and an implicit part for many years before that. It's unlikely to be changed now.
• Daniel Jour: @Edward Indeed, the only objective reason against the omission (a platform treating 0 not as successful result) is that unlikely so that it stays an subjective decision. (Which is something I think is missing from the answer, but I didn't downvote)
• edmz: Severely prefer std::string_view (currently in std::experimental but already available with modern implementations) over raw pointers for string literals: not all compilers are able to take away the lack of constexpress-ness of std::string, thus generating the relevant exception handling code.

Answer 3

Let's start with your class a. First, a is a meaningless name--because the class itself is basically meaningless. There's no purpose in the class itself--or at least no purpose in having an object of that class. It's just a collection of (somewhat) similar functions. Java doesn't give you an alternative, so those functions have to go in a class. C++ provides alternatives. The most obvious would be a namespace:

namespace math { 
 double add(double a, double b) {
 return a + b;
 }
 double sub(double a, double b) {
 return a - b;
 }
 double mult(double a, double b) {
 return a * b;
 }
 double div(double a, double b) {
 return a / b;
 }
}

Note, however, that the standard library already has (templated) versions of the same, so you probably just want to use those instead of defining your own.

Next let's consider your text class. Again, this looks like it's more a namespace than a real class--it's not really an object, just some related functions (so it should probably be a namespace).

There's also a lot of repetition in those functions--your text::add, text::sub text::div and text::mult are all identical save for one character. I'd consider something like:

string make_string(double a, double b, double ans, char op) {
 return to_string(a) + " " + op + " " + to_string(b) + " = " + to_string(ans);
}
string add(double a, double b, double ans) {
 return make_string(a, b, ans, '+');
}
string sub(double a, double b, double ans) {
 return make_string(a, b, ans, '-');
}
string div(double a, double b, double ans) {
 return make_string(a, b, ans, '/');
}
string mult(double a, double b, double ans) {
 return make_string(a, b, ans, '*');
}

I'd also separate the other functions here from the preceding. The only relationship seems to be "they all produce strings", which doesn't strike me as meaningful. Using old terminology, the cohesion here is quite low.

Finally, we can look at main, where most of the real logic is. I'm not particularly excited about it either. The variable names a and b are pretty close to meaningless. ans is a little better, but not a whole lot.

Looking at the code overall, it seems to me that the division into functions is even more problematic. Using more of that old terminology, the code is tightly coupled.

So, let's start from the opposite end of things: how would an ideal main look for this job? At least to me, "the job" seems to consist of three phases: collect inputs, process the data, display the result. We probably want a convenient container for holding the data we collect, so let's start with that:

struct params { 
 enum ops { ADD, SUB, MUL, DIV};
 enum ops op;
 double left;
 double right;
 double result;
};

So then we'll have a get_params (or something on that order) that gets the parameters from the user. It'll create a params structure. That'll get passed to a process_data (or maybe some less generic name) function. Finally, the result will get passed to a print_result function (again, or maybe something less generic).

With those, we'd get a main that looked something like this:

int main() { 
 params p = get_params();
 process_data(p);
 print_result(p);
}

Take careful note of the difference here. We now have much looser coupling--for example, if we wanted to process a string that was passed on the command line, we'd only change get_params to get input from the command line. Once that's done, process_data and print_result would remain untouched. Any change to input only affects get_params. Likewise, if we decide to change the output to (for example) write to a different destination instead of std::cout, we'd only have to change print_results().

So, the contents of get_params() would look something like this:

params get_params() { 
 params ret;
 std::cout << "Enter op: ";
 char ch = get_input("+-*/");
 switch(ch) { 
 case '+': ret.op = params::ADD; break;
 case '-': ret.op = params::SUB; break;
 case '*': ret.op = params::MUL; break;
 case '/': ret.op = params::DIV; break;
 }
 std::cout << "First operand: ";
 std::cin >> ret.left;
 std::cout << "Second operand: ";
 std::cin >> ret.right;
 return ret;
}
char get_input(std::string const &allowed) { 
 char ch;
 do {
 std::cin >> ch;
 } while (allowed.find(ch) == std::string::npos);
 return ch;
}

So, once it's done, the rest of the code can get all the input it needs wrapped up into a nice, neat little package.

From there, doing the processing becomes almost pedestrian:

void process_data(params &p) { 
 switch (p.op) { 
 case params::ADD: p.result = p.left + p.right; break;
 case params::SUB: p.result = p.left - p.right; break;
 case params::MUL: p.result = p.left * p.right; break;
 case params::DIV: p.result = p.left / p.right; break;
 }
}

Likewise, printing out the result becomes pretty trivial as well:

void print_result(params p) { 
 char names[] = "+-*/";
 std::cout << p.left << " " 
 << names[p.op] << " " 
 << p.right << " = "
 << p.result;
}

Another possibility to consider would be to write print_result as an overload of operator<< so instead of print_result(p) we'd just do something like: std::cout << p; Given your current level of experience with C++, however, it may be best to leave that for later.

• user85459: I just learned that declarations should be done in header files and definitions in cpp files. With that said, if I am going to put a set of functions and variables in a namespace, should I have the namespace in the h file or the cpp file?