critique my code?

[tstepka]

Hello!
My name is Tyler.
I have been coding for about 3 days now and am pretty much self teaching. I found an article on here about beginner exercises (http://www.cplusplus.com/forum/articles/12974/) and I thought the 'soda machine' exercise sounded pretty fun, so here is my attempt on that. Now keep in mind I do not know how to use 'switch' statements and didn't know how to use 'while' loops before I started. The only other thing I've coded before this was a simple 4 function calculator.
Please let me know if its any good!
Thanks!

  #include "stdafx.h"
#include <iostream>

void skipline();//kinda BS function for purtyness?

int main()
{
	std::cout << "Welcome to The Soda Machine!" << std::endl;
	skipline();
	std::cout << "Please make a selection" << std::endl;
	skipline();
	std::cout << " 1 = Coke		-$1.00\n 2 = Pepsi		-$1.25\n 3 = Sprite		-$1.50\n 4 = Mountain Dew	-$2.00" << std::endl;
	int(soda);
	std::cin >> soda;
	std::cin.ignore();

	//makes user choose 1-4
	while (true){
		if (soda == 1){
			std::cout << "You have chosen: Coke" << std::endl; break;
		}

		else if (soda == 2){
			std::cout << "You have chosen: Pepsi" << std::endl; break;
		}
		else if (soda == 3){
			std::cout << "You have chosen: Sprite" << std::endl; break;
		}
		else if (soda == 4){
			std::cout << "You have chosen: Mountain Dew" << std::endl; break;
		}
	else if (soda != 1, 2, 3, 4)
		std::cout << "Please make a selection 1-4" << std::endl;
		std::cin >> soda;
		std::cin.ignore();

}
	skipline();
	std::cout << "Please enter your payment below" << std::endl;
	skipline();

	float(payment);
	float(change);
	std::cin >> payment;
	std::cin.ignore();

	while (true){//makes sure user doesn't 'insert' too much moneys
		if (payment >= 10.00){
			std::cout << "Sorry this machine only takes coins, ones, and fives.\n Thanks!" << std::endl;
			std::cin >> payment;
			std::cin.ignore();
		}
		else if (payment <= 9.99){ break; }

	}
			if (soda == 1)
				change = payment - 1.00;
			else if (soda == 2)
				change = payment - 1.25;
			else if (soda == 3)
				change = payment - 1.50;
			else if (soda == 4)
				change = payment - 2.00;
			skipline();
			std::cout << "Here is your change:	$" << change << std::endl;
	
	return 0;
}

void skipline()
{
	std::cout << "" << std::endl;
}

[Blackhart98]

Pretty good so far but you could change somethings.

Line 32: you could change the whole loop to case. Then instead of

else if (soda != 1, 2, 3, 4)
		std::cout << "Please make a selection 1-4" << std::endl;
		std::cin >> soda;
		std::cin.ignore();

You could have a case loop like
switch(itemMenu)
{
case 1:
// code
break;
case 2:
// code
break;
}
Then have an default: in there.
There is a whole lot of changes that could be done but good so far.

[CodeWriter]

I am also a beginner with C++.
I am using the codeblocks ide and mingw. I am having continual problems with the compiler not being able to find an include file.
I selected a console project template at the start and pasted tstepka's code into the main.cpp but when I run it I get,
fatal error: stdafx.h: No such file or directory
.

[Blackhart98]

@Codewriter Try taking off #include stdafx.h then. I have no idea why he actually included it in his main program anyways. Probably run on his compiler only try running it now.

[CodeWriter]

@Blakchart98
Worked but gave build warnings:
else if (soda != 1, 2, 3, 4)
three warnings left / right operand of comma operator has no effect

Also I added
using namespace std;

So std::cout etc became simple cout

I still need to figure out the mechanics to allow include stdafx.h for future reference.
For example with SDL codeblocks provides a template project which works but it doesn't include the image and tff extensions. When I mindlessly follow the instructions for setting them up it fails.

I have been a BASIC programmer where graphics comes as part of the package no setting up required.

[Alrededor]

Line 32:

else if (soda != 1, 2, 3, 4)

This does not do what you want. The comma operator takes two operands. It evaluates the left-hand expression and discards the result. It then evaluates the right-hand expression and returns the result. Hence 1, 2, 3, 4 is evaluated as follows:

1,2 The left-hand expression evaluates to 1 which is discarded. The right-hand expression evaluates to 2 which is returned, leaving us with 2, 3, 4

Now we evaluate 2,3 which returns 3, leaving us with 3, 4

Finally evaluate 3, 4 which yields 4 so the condition in the if statement reduces to soda != 4

You need to compare each number separately and then join them together using the logical AND operator (&&). Change the condition in the if statement to:

else if (soda != 1 && soda != 2 && soda != 3 && soda != 4 )

Finally, lines 33-35:

Only line 33 is part of the if statement on line 32. Lines 34 and 35 are just part of the while and not the if statement. If you intended lines 34 and 35 to be part of the if statement you need to enclose lines 33-35 in braces. If you intended lines 34-35 to be outside the if and just at the end of the while, then the indentation is misleading. The code will actually work correctly either way since if you reach line 32 the condition will always be true and lines 33-35 will all execute whether inside the if statement or not. Think about it. If execution reaches line 32 it means you did not break out of loop earlier in the if else chain so soda cannot be 1, 2, 3 or 4, otherwise you would have already exited the loop. If line 32 executes the condition will always be true. Hence line 32 is not needed.

	//makes user choose 1-4
	while (true){
		if (soda == 1){
			std::cout << "You have chosen: Coke" << std::endl; break;
		}

		else if (soda == 2){
			std::cout << "You have chosen: Pepsi" << std::endl; break;
		}
		else if (soda == 3){
			std::cout << "You have chosen: Sprite" << std::endl; break;
		}
		else if (soda == 4){
			std::cout << "You have chosen: Mountain Dew" << std::endl; break;
		}
	else if (soda != 1, 2, 3, 4)
		std::cout << "Please make a selection 1-4" << std::endl;
		std::cin >> soda;
		std::cin.ignore();

} 

I have not looked for any other errors.

[Alrededor]

Another thought about your skipline function:

void skipline()
{
	std::cout << "" << std::endl;
}

You do not need to output an empty string. You can just output std::endl.

void skipline()
{
	std::cout << std::endl;
}

[TheIdeasMan]

CodeWriter wrote:

Also I added
using namespace std;

So std::cout etc became simple cout

That is actually a bad idea - It brings in the entire std namespace which is huge, all of the STL containers, classes and algorithms are in there. It also can cause naming conflicts - which is the opposite to the purpose of having namespaces.

The way the OP is doing it, is the best way. If you look at code posted by expert members of this site, they always prefix std things with std::

@tstepka

Try to avoid infinite loops like you have on lines 18 & 47. Can you think of a conditional for those while loops, that will make it end correctly?

Also, once you have a switch as Blackhart98 has suggested, I would be inclined to make each case call a separate function. This will avoid the need for the repeated code with another switch or if statements.

The code to check if too much money is put in, could be a function of it's own.

With long std::cout statements, there are a couple of things that can be done so you don't have a really long line of code:

1. the '\t' character is a tab character;
2. One can build up the output over several lines.

std::cout << " 1 = Coke\t-$1.00\n 2 = Pepsi\t-$1.25\n ";
std::cout << " 3 = Sprite\t-$1.50\n 4 = Mountain Dew	-$2.00" << std::endl;

Hope this helps out a bit :+)

[CodeWriter]

@Blackhart98
Just read in a C++ tutorial that stdafx.h always has to be used with console programs created with Visual Studio.

@TheIdeasMan
Ok. I will use std:: from now on.

[mutexe]

Just read in a C++ tutorial that stdafx.h always has to be used with console programs created with Visual Studio.

This is wrong.

[dhayden]

[using namespace std] brings in the entire std namespace which is huge, all of the STL containers, classes and algorithms are in there.

That's not true. All it does is make the symbols in namespace std that you've declared (via #includes) accessible without the std:: prefix. So if you don't have #include <vector> then std::vector isn't in the program. Put another way, using namespace std does not include all of the standard library header files.

It also can cause naming conflicts

This is the real reason to avoid using namespace std. In a larger program, you might include many files in the standard library and if your program contains a name that conflicts with one of them, you might get a very strange results. For example, if you have a variable called "list" then it will conflict with the container template std::list<> and you'll get some very odd compiler warnings.

If you don't like typing std:: in front of everything, then you can tell the compiler to use just the symbols that you need. For example:

using std::cout;
using std::cin;
using std::list;

[andywestken]

Alrededor wrote:

line 32 is not needed.

Actually, the line is needed, but should just be else (i.e. if it's not 1, 2, 3, or 4 then report an error.) And there are braces missing. (The indenting of the code is a bit misleading.)

(Aside: when quoting part of someone's code, you can use firstline to start numbering from the right point.)

[code firstline=29]

	else if (soda == 4){
		std::cout << "You have chosen: Mountain Dew" << std::endl; break;
	}
	else { // add braces so next three lines controlled by else
		std::cout << "Please make a selection 1-4" << std::endl;
		std::cin >> soda;
		std::cin.ignore();
        }

[/code]

Andy

PS Trivia??

int(soda);

is more usually written

int soda;

or even better,

int soda = 0;

or

int soda(0);

to initialize the variable.

[Ch1156]

Ive read your post and a few others but skimmed through the rest, your while loop you could do this:

bool loopEnd = false;

	//makes user choose 1-4
	while (!loopEnd)
    {
		if (soda == 1){
			std::cout << "You have chosen: Coke" << std::endl;
			loopEnd = true;
		}

		else if (soda == 2){
			std::cout << "You have chosen: Pepsi" << std::endl;
			loopEnd = true;
		}
		else if (soda == 3){
			std::cout << "You have chosen: Sprite" << std::endl;
			loopEnd = true;
		}
		else if (soda == 4){
			std::cout << "You have chosen: Mountain Dew" << std::endl;
			loopEnd = true;
		}
	else if (soda != 1, 2, 3, 4)
		std::cout << "Please make a selection 1-4" << std::endl;
		std::cin >> soda;
		std::cin.ignore();

    }

It's better to use continue; and break; sparingly.

[TheIdeasMan]

@dhayden

Hi, how are you - thanks for pointing out this:

All it does is make the symbols in namespace std that you've declared (via #includes) accessible without the std:: prefix.

++ThingsLearnt;

I guess that I make comments against using an entire namespace routinely, especially when the OP was arguably doing it in the normally accepted way - I don't like to see someone promoting the "wrong way".

Without trying to be argumentative, it's quite easy to write even a small amount of code that would include a fair bit of stuff from std, as in:

#include <iostream>
#include <string>
#include <vector>
#include <cmath>
#include <algorithm>
#include <memory> 

As you mention naming conflict is easily done, even with just <iostream>, there is std::left and std::right , which are used for alignment purposes via std::cout, a beginner could easily try to use those as variable names.

It still seems strange that so many authors & lecturers (including Stroustrup) have using namespace std;. Even in a Hello World program - the meaning of namespace seems to be glossed over, if not being promoted as being easier.

Anyway, I guess there are bigger things in the world to worry about :+)

@Ch1156

With your loop, and setting loopEnd after each else if, the loop only runs once. I would rather there be some sort of Quit option the sets loopEnd. That way the user can repeatedly make selections.

Also, you missed the error on line 23.

As others have pointed out the else part should be used to catch errors, and analogous to the default: case in a switch

It's better to use continue; and break; sparingly.

Not sure what you mean by this.

The OP had them because of the use of the infinite loops - maybe the overuse of them points towards better design (like the use of the boolean in the loop), but otherwise I don't see why one shouldn't use them where ever needed.

Hope all is well with everyone - have a great day :+)

[CodeWriter]

codeWriter wrote:
Just read in a C++ tutorial that stdafx.h always has to be used with console programs created with Visual Studio.

mutexe replied:
This is wrong.

Ok. My source was C++ Without Fear, chapter 1, "If You're Using Microsoft Visual Studio".
Not that it matters as I am using codeblocks.

[cire]

TheIdeasMan wrote:

[using namespace std] brings in the entire std namespace which is huge, all of the STL containers, classes and algorithms are in there.

dlhayden wrote:

That's not true. All it does is make the symbols in namespace std that you've declared (via #includes) accessible without the std:: prefix. So if you don't have #include <vector> then std::vector isn't in the program. Put another way, using namespace std does not include all of the standard library header files.

Since one cannot depend on a standard header not including any other standard header, it really is potentially true. Then you have to figure you may be including some other header that may also include other standard headers and you've got me scratching my head a little here. Why would you bother with this distinction? It's completely useless and seems to say "Hey, using directives are completely predictable and not prone to causing problems at all, so go ahead and use them indiscriminately," which, of course, is not true.

[tstepka]

Thanks for all the input guys! Like I stated prior I've only been at this maybe 8 hours total and just now read about switch and case. I am using visual studios as recommended by the tutorial on learncpp..com. I will try and modify this program as I get time(not much of that anymore since being married)

[dhayden]

Since one cannot depend on a standard header not including any other standard header, it really is potentially true.

That's true. I mainly wanted to make the point to avoid having someone think "hey, if I add using namespace std; then I don't need all those pesky include files because it "brings in" the whole std namespace.