Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

Putting using namespace std at the top of every program is a bad habit a bad habit that you'd do well to avoid.

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Source Link
Edward
  • 67.2k
  • 4
  • 120
  • 284

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

Fix your formatting

The code as posted has inconsistent indenting which makes it hard to read. Choose a particular style and apply it consistently to make your code easier to read.

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.

Validate the input

The code does not seem to be able to handle negative numbers as in the expression:

-3+2

If it's not intended to handle negative numbers, it would be good to let the user know. If it is, then there's a bug that should be fixed.

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Don't use system("PAUSE")

There are two reasons not to use system("cls") or system("PAUSE"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named PAUSE or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions pause() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++. For example:

void pause() {
 getchar();
}
 

General portability

This code could be made portable if you omit the Windows-only include files #include "stdafx.h". If you must have stdafx.h, consider wrapping it so that the code is portable:

#ifdef WINDOWS
#include "stdafx.h"
#endif

Separate input, output and calculation

To the degree practical it's usually good practice to separate input, output and calculation for programs like this. By putting them in separate functions, it isolates the particular I/O for your platform (which is likely to be unique to that platform or operating system) from the logic of the program (which does not depend on the underlying OS). So for example, the main program might call a function to fetch and validate the input, a second function to actually perform the mathematical operations and then a third to emit the results.

Omit spurious parentheses

The argument to a case statement or return statement does not need parentheses, so instead of this:

 case('#'):

One could write this:

 case '#':

Consolidate strings

Right now, the string "Input not recognized" appears four times in the program. I'd recommend creating a const variable instead and then using that instead of repeating the string.

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /