current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

I have been learning C++ programming. What you see here is all that I know how to do at this point. I look forward to learning more and I know there are better ways to accomplish a task such as calculating, or a less verbose method for displaying information.

I want to hear from more experienced developers and engineers what they would have done better for the following code.

As I am learning new techniques I will be more inclined to pick up the better, more efficient methods of accomplishing a task, however simple it may be. I have heard that it is common for beginners to harbor bad habits. It would be the greater good as you may have to work with the likes of me in the future.

#include <iostream>

using namespace std;

int main()
{
int a;
int b;
int sum;
int diff;
int prod;
int quot;
int remain;

cout << "Welcome to the Basic Calculator for integers!" << endl;
cout << "Enter two numbers to see the difference, and please: one at a time!" << endl;

cout << "First Number: ";
cout.flush();
cin >> a;

cout << "Second Number: ";
cout.flush();
cin >> b;
cout << "\n";

sum = a + b;
diff = a - b;
prod = a * b;

quot = a/b;
remain = a%b;

cout << "Basic calculations for your two numbers: \n" << "Sum: " << sum << "\n" << "Difference: " << diff << "\n" << "Product: " << prod << "\n" << "Quotient: " << quot << " with a remainder of " << remain << endl;
}

I know that my white space looks terrible and indenting is a good thing, but for ease of formatting it looks a little uglier than I would like.

share|improve this question
    
Thanks for the information. –  brndng yesterday

2 Answers 2

up vote 7 down vote

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

Decompose your program into functions

All of the logic here is in main in one chunk of code. It may be better to decompose this into separate functions, such as separate ones for input, calculation and output.

Declare variables as late as possible

Rather than using the old C-style of declaring all variables at the top of a function, use the modern C++-style and declare variables as late as possible. Doing so can sometimes help the compiler figure out register allocation, resulting in faster, smaller code.

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. It is particularly bad to put it into a header file, so please don't do that.

Use constant string concatenation

This code currently starts with these two lines:

cout << "Welcome to the Basic Calculator for integers!" << endl;
cout << "Enter two numbers to see the difference, and please: one at a time!" << endl;

The two observations I would make are that the first one doesn't really need endl because just a new line will do. (The difference is that std::endl additionally flushes the buffer.) The second observation is that you don't really need to call << four times. Instead, you can use string concatenation and call it only twice:

cout << "Welcome to the Basic Calculator for integers!\n"
    "Enter two numbers to see the difference, and please: one at a time!" << endl;

Sanitize user input better

The code has a potential problem as posted. If I enter a string such as "Edward" to instead of a number, the program stays in an endless loop. It would be better to read a (text) line in and then convert it to a number. Users can do funny things and you want your program to be robust.

Pay attention to mathematical problems

If the user chooses zero for the second number, the program is likely to crash because of a divide-by-zero. Your code should check the input for zero.

share|improve this answer
    
Thank you for such a complete review! I am going to practice string concatenation and declaring variables close to where they are being used. On the topic of sanitizing user input: Would you be able to provide an example of how the program could better receive user input? I am so new to this that I don't quite know how to catch exceptions. –  brndng yesterday
    
See codereview.stackexchange.com/questions/105088/… for some ideas on how to sanitize user input. –  Edward yesterday
up vote 1 down vote

My biggest recommendations are:

  1. Variable names should fully describe what they represent - "a", "b", "x", etc. should not be generally used, unless you're doing code golf. Also, "diff" should be "difference", "prod" should be "product". This makes your code much easier for others to read and maintain.

  2. Declaring separate variables to hold the value of each calculation is ok, but I would recommend only holding the values that you are manipulating, and not creating variables just for output purposes.

    For instance, instead of the following:

    cout << sum;

    You could do:

    cout << firstNumber + secondNumber << endl;

    This would save you from having to allocate the memory for the sum variable for example, as well as the extra keystrokes necessary to assign a value to the sum variable and then output that value. You would be saving memory and time spent, which results in a productivity increase.

share|improve this answer
4  
Completely disagree with (2). Store it in a seprate variable will never hurt (as the optimizer will remove the need for the variable if it is not used). But during debugging it can be useful to see intermediate values in the debugger. So it never hurts and sometimes comes in handy. Also it makes the code more readable for the maintainer so it makes it easier to maintain. –  Loki Astari yesterday

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.