code review request: simple adder

[EJW]

This is a simple academic adding program. It inputs an integer, then an operator, then a second integer, and calculates and prints the result.

I want to improve it by enabling the user to input a string and then have my program parse it into an arithmetical expression.

Also my typechecking in function getInteger() doesn't seem robust enough. It can't reject trailing garbage such as "25bla" where I want it to reject the input because of the trailing "bla." If I can have some help with that it would be appreciated.

stringformat.h

#ifndef STRINGFORMAT_H
#define STRINGFORMAT_H

const char newln { '\n' };
const char tab   { '\n' };
const char bs    { '\b' };

#endif 

main.cpp

#include "stringformat.h"
#include <cstdlib>
#include <string>
#include <iostream>

using namespace std;

int getInteger(std::string prompt)
{
	float integer { };

	while (true)
	{
		cout << prompt;
		cin >> integer;
		if (cin.fail())
		{
			cin.clear();
			cin.ignore(32767, newln);
			cout << "Not a number." << newln;
		}
		else if (integer != static_cast<int>(integer))
		{
			cin.clear();
			cin.ignore(32767, newln);
			cout << "Not an integer." << newln;
		}
		else
		{
			cin.clear();
			cin.ignore(32767, newln);
			return static_cast<int>(integer);
		}
	}
}

char getOperator(std::string prompt)
{
	char operrator { };

	while (true)
	{
		cout << prompt;
		cin >> operrator;
		if (operrator == '+' || operrator == '-' || operrator == '*' ||
			operrator == '/' || operrator == '%')
		{
			cin.clear();
			cin.ignore(32767, newln);
			return operrator;
		}
		cout << "Invalid choice--enter =, -, *, /, or %. " << newln;
	}
}

int evaluate(int first_integer, char operrator, int second_integer)
{
	if (operrator == '/' && second_integer == 0)
	{
		cout << "Can't divide by zero." << newln;
		exit(0);
	}

	switch (operrator)
	{
		case '+':
			return first_integer + second_integer;
		case '-':
			return first_integer - second_integer;
		case '*':
			return first_integer * second_integer;
		case '/':
			return first_integer / second_integer;
		case '%':
			return first_integer % second_integer;
		default:
			cout << "Expression unevaluable.";
			exit(0);
	}
}

void printResult(int first_integer, char operrator, int second_integer,
				 int result)
{
	cout << first_integer << " " << operrator << " " << second_integer
		 << " = " << result << '.' << newln;
}

int main()
{
	int first_integer  { getInteger("Enter the first integer: ") };
	char operrator     { getOperator ("Enter the operator (=.-, *, /): ") };
	int second_integer { getInteger("Enter the second integer: ") };
	int result         { evaluate(first_integer, operrator, second_integer) };
	printResult(first_integer, operrator, second_integer, result);

	return 0;
}

[jlb]

You may want to consider getting your input as a std::string, then parse that string looking for "bad" characters.

By the way is "2 * 3(enter key)" considered a good or bad entry?

Also a "tab" character is '\t' not '\n'.

[Duthomhas]

It is well-thought and well-organized. However, there are a few problems that you need to fix.


The first is that you are declaring data in a header. Don’t do that..

Headers exist to tell other .cpp files (“translation units”) what exists in a specific .cpp file. (Or provide templates to tell the compiler how to create stuff elsewhere.)

They do not exist to create data.


The second is that you have fallen into the ‘redefine trap’, as I call it — an attempt to prettify or in some way get around simply using stuff.

There is absolutely no need to declare constants to refer to newline, tab, backspace, or any other value that has well-defined meaning, such as '\n', which is always a newline. Simply include it as part of your output string:

  cout << "Not a number.\n";

Combined with that you are using a magic number (32767) that you should be getting from the compiler properly.

If you find that you need to cin.clear() and cin.ignore() frequently, consider writing a little inline function for your convenience:

inline void cin_fix()  // reset_user_input(), cin_clear_and_ignore(), skip_bad_input(), etc.
{
  std::cin.clear();
  std::cin.ignore( std::numeric_limits <std::streamsize> ::max(), '\n' );
}

Don’t name objects with the names of types. It is confusing, especially if you have to resort to mis-spelling things to do it. “value” is a good name. As is “operator_name”. Etc.

Of course, if you just use something like “n” or “x” for the input value base name, and “op” for the operator name, that would follow very easily in time-honored naming conventions that everyone understands.


This next is my personal opinion, but avoid the weird {} syntax for variable initializations. Use the assignment operator; the compiler understands that you are declaring and initializing a value and will do the right thing.

int main()
{
	int  n1     = getInteger("Enter the first integer: ");
	char op     = getOperator ("Enter the operator (=.-, *, /): ");
	int  n2     = getInteger("Enter the second integer: ");
	int  result = evaluate(n1, op, n2);

	printResult(n1, op, n2, result);
}

This is much more readable.


Trim unnecessary functions: printResult(). It doesn’t do anything but print its arguments. But... that’s what cout does!

int main()
{
	int  n1     = getInteger("Enter the first integer: ");
	char op     = getOperator ("Enter the operator (=.-, *, /): ");
	int  n2     = getInteger("Enter the second integer: ");
	int  result = evaluate(n1, op, n2);

	cout << n1 << " " << op << " " << n2 " = " << result << "\n";
}

That is even more readable than before.


The problem you are having with getInteger() is that you are not accepting user input as a string. By trying to get a float == integer you are asking for problems.

A good rule of thumb: if you ask the user to type something, they will always give you a string and press ENTER.

Once you get that string, then you should attempt to convert it to what you asked for. If conversion works, all is good. Otherwise complain and quit!.

Fairly, getting user input properly is not as easy as people like to think or pretend.

#include <sstream>

int getInteger(const std::string& prompt)
{
  std::string s;
  std::cout << prompt;
  if (!getline( std::cin, s )) complain("Unexpected EOF");

  std::istringstream ss( s );
  int result;
  ss >> result >> std::ws;
  if (!ss.eof()) complain("Not an integer");

  return result;
}

Whatever you do to “complain” is up to you. Throwing a runtime_error is a good idea — the program will terminate and print your error to the user. This should be the case for all spots that produce an error and exit().


Lines 58–62 belong down with line 73: keep special processing with the object that handles it.

		case '/':
			if (second_integer == 0) complain("You cannot divide by zero.");
			return first_integer / second_integer;

Also, you could get rid of using namespace std;. If you don’t, though, you should try to be consistent about using “std::” prefixes.


Well, I hope this helps.

[edit] Fixed typo