2nd advance program

[Program Programmer]

#include <iostream>
#include <cstdlib>
using namespace std;
int main ()
{
 system("color 3f");
 int choice1;
 int choice2;
 int choice3;
 int choice4;
 int choice5;
 int choice6;
 int choice7;
 int choice8;
 int choice9;
 int result;
 int result2;
 int result3;
 int result4;
 for (;;) {
 do {
 
 cout << "-----------------------------\n";
 cout << "|         calculator        |\n";
 cout << "|   1 for +                 |\n";
 cout << "|   2 for -                 |\n";
 cout << "|   3 for /                 |\n";
 cout << "|   4 for *                 |\n";
 cout << "|   5 to quit               |\n";
 cout << "-----------------------------\n";
 cin >> choice1;
 } while (choice1 == '1' &&choice1 == '2' &&choice1 == '3' &&choice1 == '4' &&choice1 != '5');
  if (choice1 == '5') break;
  
  switch (choice1) {
  
   case 1:
   
    cout << "Plus. Enter two numbers: \t";
	cin >> choice2;
	cin >> choice3;
	
	result = choice2 + choice3;
	
	cout << "result: \t" << result << "\n";
	
	break;
	
   case 2:
   
    cout << "Minus. Enter two numbers: \t";
	cin >> choice4;
	cin >> choice5;
	
	result2 = choice4 - choice5;
	
	cout << "result: \t" << result2 << "\n";
	
	break;
	
   case 3:
   
    cout << "Divide. Enter two numbers: \t";
	cin >> choice6;
	cin >> choice7;
	
	result3 = choice6 / choice7;
	
	cout << "result: \t" << result3 << "\n";
	
	break;
	
   case 4:
   
    cout << "Multiplication. Enter two numbers: \t";
	cin >> choice8;
	cin >> choice9;
	
	result4 = choice8 * choice9;
	
	cout << "result: \t" << result4 << "\n";
	
	break;
	
   case 5:
	
   return 0;
	
   default:
   
   cout << "That is not a valid option.\n";
   
  
   
}

}

return 0;

}

what do you think

[LB]

Line 3: You know this is not good practice, correct? It's fine for small things but you should get out of the habit.
Lines 7-19: o_o what is all this for?
Lines 25-29: Can't I just type an actual +, -, *, or / instead of pressing a number?
Line 32: I do not think this does what you think it does.
Lines 35-83: You don't need a different set of variables for each case, you can just re-use the same "a" "b" "result" variables in each one, since they get overwritten anyway.
Line 87: It's bad practice to have more than one exit point from an application. This also doesn't make sense because of Line 33...
Line 91: if your while loop in line 32 worked properly then this would never execute.

[Program Programmer]

this work just fine for me. no bugs. not even a tiny one.

[LB]

Why would you ask what I think and then ignore all my thoughts?

[3TXyhbRD]

I think it's ok except that your code is hardly formatted (means it's hard enough to read and follow). Also line 20 is a bad choice from my point of view. Apart from being a for (means you know the number of iterations at that point) it's an infinite loop. Also your main function is massive + what L B said.

In conclusion it needs polishing.

[Program Programmer]

so if i removed line 20, it would stop looping when you type letters?

[MaxterTheTurtle]

Hmm, this isn't an "Advanced Program." Its good for a beginner. I remember when I was making small text RPGS with GOTO statements. Life was simpler back then.

1.) For instance you don't use arrays -- or as others said OOP.
2.) Also as others said, your main functions is massive.
3.) The use of System is bad practice and should be avoided, unless used properly. You should try http://msdn.microsoft.com/en-us/library/windows/desktop/ms686047%28v=vs.85%29.aspx instead of System("color 3f");
4.) Also, instead of using a switch/case statement you should try using functions.
5.) Lastly, it seems strange to me that the statement that determines whether to break the loop is comparing an integer to a char. Also, I would do something like

do
{
//Code goes here
}
while (choice >= 1 && choice <= 4)

[Program Programmer]

i just understood, uh, none of that.

[Lowest0ne]

you let the user divide by zero.

Line 87: It's bad practice to have more than one exit point from an application. This also doesn't make sense because of Line 33...

Really? I do things like this all the time:

if (argc != 2) return 0;

open file(argv[1])
if (file isn't open) return 0;
... 

[Program Programmer]

yeah, and also, break; doesn't work for me.

[MaxterTheTurtle]

You should finish the tutorials on this website. What I said wasn't too hard to understand.

OOP - Object Oriented Programming
http://www.cplusplus.com/doc/tutorial/classes/

It is bad practice to have all of your code in the int main () function. So use functions and you could name the functions add,subtract,multiply,divide,etc.
http://www.cplusplus.com/doc/tutorial/functions/

Using system(); is considered bad practice, as it leaves security bugs in the program. (I had a link, but I can't find it anymore.) Based on the fact that you are using a cmd color changer, I am assuming you are using windows. In that case use:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms686047%28v=vs.85%29.aspx

Here is some code that I wrote, to demonstrate how to use it. (with the help of the forum of course):

/* include the input/output stream for console */
#include <iostream>

/* include the windows header for windows api functions */
#include <Windows.h>

/* declare a function to change the console window color */
void v_changeConsoleColor(int color)
{
	/* get the current handle of the command prompt window */
	HANDLE handle = GetStdHandle(STD_OUTPUT_HANDLE);

	/* assign the color of the text in the command prompt window */
	SetConsoleTextAttribute(handle, color);
}

/* the main entry point */
int main (void)
{
	/* output hello world into the command prompt window */
	std::cout << "Hello World!" << std::endl;

	/* change the color of the window */
	v_changeConsoleColor(2);

	/* output hello world into the command prompt window */
	std::cout << "Hello World Again!" << std::endl;

	/* wait for user input */
	std::cin.get();

	/* return success */
	return 0;
}

In your code:

do
{
//Code goes here
}
while (choice1 == '1' &&choice1 == '2' &&choice1 == '3' &&choice1 == '4' &&choice1 != '5');

Seems like bad practice to me, since you are comparing integers to characters. I would try:

do
{
//Code goes here
}
while (choice >= 1 && choice <= 4)

Edit:
The break; statement only works in loops such as while, do-while, and for loops.

I'm not trying to be mean, but you asked for our feedback, and now you criticize what we give you.

[htirwin]

 do {
 
 cout << "-----------------------------\n";
 cout << "|         calculator        |\n";
 cout << "|   1 for +                 |\n";
 cout << "|   2 for -                 |\n";
 cout << "|   3 for /                 |\n";
 cout << "|   4 for *                 |\n";
 cout << "|   5 to quit               |\n";
 cout << "-----------------------------\n";
 cin >> choice1;
 } while (choice1 == '1' &&choice1 == '2' &&choice1 == '3' &&choice1 == '4' &&choice1 != '5');

This doesn't make sense. It's impossible that your loop condition will be true.

It's equivalent to,

 cout << "-----------------------------\n";
 cout << "|         calculator        |\n";
 cout << "|   1 for +                 |\n";
 cout << "|   2 for -                 |\n";
 cout << "|   3 for /                 |\n";
 cout << "|   4 for *                 |\n";
 cout << "|   5 to quit               |\n";
 cout << "-----------------------------\n";
 cin >> choice1;
 

[TheIdeasMan]

Psuedocode for a basic calculator:

//Get a number
//Get an operator
//Get a number
//calculate result
//print result on screen 

This immediately shows you some functions you should have. This will save some repetition in your code.

Use a switch to process the operators.

A menu doesn't seem necessary.

Put the whole thing in a loop, so you can do multiple calculations - use a bool variable for this:

bool Quit = false;

while (!Quit) {
//your code here

//ask user if they want to quit
//make use of the toupper function to reduce testing

    if (toupper(Answer) == 'Q') {
      Quit = true;
    }

}

The psuedocode at the top is a good way to organise your thoughts. You can start with some really basic ideas, then keep going back and refine them by adding more detailed comments. When you are ready you can write the code - leave the comments as they serve as documentation.

HTH

[Xphysics]

Different kind of calculator. Good practice...

[JockX]

Program Programmer said:
this work just fine for me. no bugs. not even a tiny one.

Then I gues infinite loop after user inputting a letter instead of a digit is not a bug. It's a feature.

[Program Programmer]

Then I gues infinite loop after user inputting a letter instead of a digit is not a bug. It's a feature.

i wrote that before i found about that

[3TXyhbRD]

Multiple exit points in code make it more open to logical errors (bugs). Keeping the code clear and simple reduces the probability of a bug to appear (sometimes makes the probability equal to 0).

Using functions is encouraged because it features reusable code and ease of testing. For instance if you always write your code in the main function you will always need to test what's inside it (even if you copy and paste code written before). You may write a function that takes a "valid" character (valid characters can mean '+', '*' etc.) and two numbers and applies the mathematical operation for the two values. This function is highly reusable because you use it with every mathematical operation that your calculator does (except unary operations, but you can write a similar function for those).

I personally avoid break; and continue; because, from my point of view, it makes the code harder to understand (more exit points). The only place where I use break; is in switch.. case statements because the syntax demands it.

If I were you I would avoid platform dependency for now [talking about Win API] (seeing that your programming level is around beginner) because there's more to it and I would hate to rush someone into something before he understands the fundamentals of an OS, just my two cents.

@TheIdeasMan
Your code is ok-ish however it would rather be more readable and efficient to use a do.. while loop when you know you will execute, at least once, the body of a loop. For instance you code, from my point of view, is better looking and efficient if it looks like this:

bool quit = false;

do{
//your code here

//ask user if they want to quit
//make use of the toupper function to reduce testing

    if (toupper(answer) == 'Q') {
      quit = true;
    }

}while (!quit);

I also took the liberty of writing all variables in the lower camel case naming convention.