Why Wont this Code work properly?

[zapshe]

Now, I'm sure there is something really simple and stupid I just didn't realize - I just started C++ a day or so ago. The problem with this code is that no matter what I put for z (even if it's not something I wrote an "if" statement for) the process will only Multiply the 2 integers. Taking the multiplication feature out made it only divide. Why is this? Last thing to mention is that I saw a calculator made similarly but z was an integer. Instead of using the signs for mathematics for z, they made numbers represent the operations in their code. This seemed like a pain and so I wrote this code from scratch - hoping to replace integers with characters. Thanks.

  #include "stdafx.h"

#include <iostream>
#include <string>
 
int a;
int x;
int y;
char z;
int subtract()
{
	a = x - y;
	return a;
}

int add()
{
	a = x + y;
	return a;
}

int divide()
{
	a = x / y;
	return a;
}

int multiply()
{
	a = x * y;
	return a;
}
int main()
{
	std::cout << "Pick Your First Integer: ";
	std::cin >> x;
	std::cout << "Pick Your Second Integer: ";
	std::cin >> y;
	std::cout << "Choose Your Operation: ";
	std::cin >> z;
	if (z == '-');
	{
		subtract();
	}

	if (z == '+');
	{
		add();
	}

	if (z == '/');
	{
		divide();
	}

	if (z == '*');
	{
		multiply();
	}
	

	std::cout << "The Sum Is: " <<a << std::endl; 
	
} 

[zapshe]

Alright, looking over it for a millionth time - Keeping my eyes open for anything. I realized the height of my current stupidity in this matter. I put semicolons after each if (z == '?') . That made it never even finish the function that it was supposed to do. Interesting though that it would constantly do the last function on there (Multiplying) when this happened though. Thanks anyway guys.

[keskiverto]

Read http://www.cplusplus.com/doc/tutorial/functions/

Functions can take arguments and return values. That is much more flexible than the global variables that you have.



Your first actual problem is formatted input.
http://www.cplusplus.com/reference/istream/istream/operator%3E%3E/
Your lines 36 and 38 read integers.
* First they discard whitespace characters (spaces, tabs, newlines) until digits start
* Then they take each digit
* When they see a non-digit, they stop and return the number that the digits represent

The first non-digit is still in the stream. If you did hit Enter (a newline) after the first number, then that newline is skipped as whitespace by the line 38 read operation.

You probably do hit Enter after the second number too. The trick is that the >> that reads a char is different:
http://www.cplusplus.com/reference/istream/istream/operator-free/
It reads the next char. Newline is a char.
If you had hit +, -, *, /, the z would have "proper data".

Hint, you could print the value of z before line 41 to see what you actually did get.


The second problem is on lines 41, 46, 51, and 56. What is the last character on each of those lines?

Now check http://www.cplusplus.com/doc/tutorial/control/

The if has two options:

// one statement
if ( cond )
  statement ;

// multiple statements
if ( cond )
{
  statement ;
  statement ;
}

However, whitespace is quite optional. The one statement version can be a oneliner:
if ( cond ) statement ;
Statement can be empty too:
if ( cond ) ;
and trim rest of whitespace:
if( cond );
A IF-something-THEN-DO-nothing ...


Another debug hint: you can print the value of a in each function. Then you will see that they are in fact all called due to those misplaced ;'s ...

[zapshe]

Aren't lines 36/38 suppose to read integers - since they need to be numbers? Also, pressing enter for a new line doesn't change anything - at least not with my code. Removing the semicolons (;) from the lines 41/46/51/56 made the program work as it should. I typed them in since I tried to make it instinctive due to it being needed at the end of function statements - I need to be more careful where I type them. Thanks for your reply.

[keskiverto]

Sorry, you are right; the >> char version skips whitespace too.


Here are two alternatives for your independent ifs:

if (z == '-')
{
	subtract();
}
else if (z == '+')
{
	add();
}
else if (z == '/')
{
	divide();
}
else if (z == '*')
{
	multiply();
}

switch ( z )
{
case '-':
	subtract();
	break;
case '+':
	add();
	break;
case '/':
	divide();
	break;
case '*':
	multiply();
	break;
case default:
	std::cout << "unknown operator: '" << z << "'\n";
}

[zapshe]

Thanks for the reply. I've yet to use the "switch" function but I'v gotten a good idea of what it does by looking at the code. Hope you wont mind this question since I haven't researched it - But why do you need the "break" command - and why didn't you use it for the case default line? Thanks again for your help, I'm trying to learn as much as I can.

[keskiverto]

The switch jumps to the case that matches. Then it executes all lines from that point onward.

For example, if z=='/', then execution jumps from line 1 to line 9 (over lines 2-8). The 'default' is a "catch all".

A break; within scope of switch jumps out from the scope, (here to line 17). The default case reaches the end anyway, so does not need a break-out.

You could use this:

switch ( z )
{
case 'n':
case 'N':
	home();
	break;
case 'y':
case 'Y':
	bar();
	break;
case default:
	std::cout << "unknown selection: '" << z << "'\n";
}

Here both 'n' and 'N' lead to home(), both 'y' and 'Y' to bar(), and anything else is, well, unexpected.

Overall, switch seems to have less use.


There is yet another approach. A more dynamic one. Imagine a table with two columns. The first has a "key" (like the -, +, /, *). The second column has a "value".

We don't need to know the size of the table during compilation. We can fill it during runtime.

Now, rather than having ifs or a switch, we search key from the table. If we do find it, then we can pick the corresponding value.

It is possible to store a function's address in an object (in value) and call the function with that address. For example, find key '/' from table and "calling" the corresponding value executes the divide().

[zapshe]

Thanks for the response, it was very insightful. I understood the "table", but I don't know how to do that yet. I suppose it'll be something I'll learn later. Thanks again !

Edit : I forgot to mention that putting in the code previously :

switch ( z )
{
case '-':
subtract();
break;
case '+':
add();
break;
case '/':
divide();
break;
case '*':
multiply();
break;
case default:
std::cout << "unknown operator: '" << z << "'\n";
}



^ Results in an error. It's the last to 3rd line - tells me that it expected an expression after "case". Is there something missing on my end or what? Thanks!

[Ganado]

It's not case default:, but just default:

[zapshe]

Ah, thanks Ganado - That let the program run smoothly. That line of code with the switch is better since my original "if" functions - they would always show "Unknown Operation" (it's a line of code I added as an "else" statement). It was supposed to appear anytime someone put some incorrect information - but it would show up every time it finished the arithmetic. I was probably using the else statement wrong or something. Either way, the "break" function could have been what solved that - Plus it looks handy for future use. So thanks Keskiverto and Ganado !