Critique my code please?

[gers1978]

Hi, I'm a beginner at C++ and am doing Introduction To Programming at University (night school). I'm also working through Walter Savitch's Absolute C++ in my own time.

I've tackled one of the Programming Projects in chapter 4 of the book, and was wondering if someone here could look at the code and let me know where I've gone right or wrong. The code is fine in that it does what the assignment asks, but I'm sure it's pretty rubbish and could be done much better. The assignment details are in the opening comments of the code.

Many thanks.

/*Write a program that outputs all 99 stanzas of the “Ninety-Nine Bottles of Beer
on the Wall” song. Your program should print the number of bottles in English,
not as a number:

Ninety-nine bottles of beer on the wall,
Ninety-nine bottles of beer,
Take one down, pass it around,
Ninety-eight bottles of beer on the wall.
…
One bottle of beer on the wall,
One bottle of beer,
Take one down, pass it around,
Zero bottles of beer on the wall.

Your program should not use ninety-nine different output statements!*/

#include <iostream>
#include <string>

using std::cout;
using std::cin;
using std::endl;
using std::string;

string funcFirstDigit(int, int);
string funcSecondDigit(int);

int main()
{
	int firstDigit, secondDigit, bottles = 99;
	string line1, line2, line3, line4, altLine1, altLine2, altLine4, secondDigitText, ones, twenty, thirty, forty, fifty, sixty, seventy, eighty, ninety;
		
	altLine1 = " bottle of beer on the wall,"; //used when down to 1 bottle rather than use plural
	altLine2 = " bottle of beer,"; //used when down to 1 bottle rather than use plural
	line3 = "Take one down, pass it around,";
	altLine4 = " bottle of beer on the wall."; //used when down to 1 bottle rather than use plural
				
	for(int a = bottles; a >= 0; a--)
	{		
		firstDigit = bottles / 10; //get the first digit of the number of bottles remaining
		secondDigit = bottles % 10; //get the second digit of the number of bottles remaining
		line1 = " bottles of beer on the wall.";
		line2 = " bottles of beer,";
		line4 = " bottles of beer on the wall.";
				
		//specify correct text rather than a blank string when bottles reached zero
		if(bottles != 0) secondDigitText = funcSecondDigit(secondDigit);
		else secondDigitText = "Zero";
									
		//examine the first digit and display the correct output.  Calls function to calculate text to display for 2nd digit
		switch(firstDigit)
		{
			case 0:
			{
				//use singular version if only 1 bottle left
				if(funcSecondDigit(secondDigit) == "One")
				{
					line1 = altLine1;
					line2 = altLine2;
				}
				cout << secondDigitText << line1 << endl << secondDigitText <<
				line2 << endl << line3 << endl << secondDigitText << line4 << endl;
				break;
			}
			case 1:
			{
				ones = funcFirstDigit(firstDigit, secondDigit);
				cout << ones << line1 << endl << ones << line2 << endl << line3 << endl << ones << line4 << endl;
				break;
			}
			//in cases 2 through 9, "if" statement required as we don't need the dash if the remaining bottles is divisible by 10
			case 2:
			{
				if(secondDigit == 0) twenty = "Twenty";
				else twenty = "Twenty-";
				cout << twenty << secondDigitText << line1 << endl << twenty << secondDigitText <<
				line2 << endl << line3 << endl << twenty << funcSecondDigit(secondDigit) << line4 << endl;
				break;
			}
			case 3:
			{
				if(secondDigit == 0) thirty = "Thirty";
				else thirty = "Thirty-";
				cout << thirty << secondDigitText << line1 << endl << thirty << secondDigitText <<
				line2 << endl << line3 << endl << thirty << secondDigitText << line4 << endl;
				break;
			}
			case 4:
			{
				if(secondDigit == 0) forty = "Forty";
				else forty = "Forty-";
				cout << forty << secondDigitText << line1 << endl << forty << secondDigitText <<
				line2 << endl << line3 << endl << forty << secondDigitText << line4 << endl;
				break;
			}
			case 5:
			{
				if(secondDigit == 0) fifty = "Fifty";
				else fifty = "Fifty-";
				cout << fifty << secondDigitText << line1 << endl << fifty << secondDigitText <<
				line2 << endl << line3 << endl << fifty << secondDigitText << line4 << endl;
				break;
			}
			case 6:
			{
				if(secondDigit == 0) sixty = "Sixty";
				else sixty = "Sixty-";
				cout << sixty << secondDigitText << line1 << endl << sixty << secondDigitText <<
				line2 << endl << line3 << endl << sixty << secondDigitText << line4 << endl;
				break;
			}
			case 7:
			{
				if(secondDigit == 0) seventy = "Seventy";
				else seventy = "Seventy-";
				cout << seventy << secondDigitText << line1 << endl << seventy << secondDigitText <<
				line2 << endl << line3 << endl << seventy << secondDigitText << line4 << endl;
				break;
			}
			case 8:
			{
				if(secondDigit == 0) eighty = "Eighty";
				else eighty = "Eighty-";
				cout << eighty << secondDigitText << line1 << endl << eighty << secondDigitText <<
				line2 << endl << line3 << endl << eighty << secondDigitText << line4 << endl;
				break;
			}
			case 9:
			{
				if(secondDigit == 0) ninety = "Ninety";
				else ninety = "Ninety-";
				cout << ninety << secondDigitText << line1 << endl << ninety << secondDigitText <<
				line2 << endl << line3 << endl << ninety << secondDigitText << line4 << endl;
				break;
			}
		}
						
		bottles--;
		cout << endl; //separate each verse with a blank line
	}
			
	return 0;
}

//bring in first digit from number of bottles remaining to check for special case of teens, etc
//returns that digit in correct text form	
string funcFirstDigit(int firstDigit, int secondDigit)
{
	string text;
		
	if(firstDigit == 1) //only need to do the following if the number of bottles left is between 10 and 19 (inclusive)
	{
		switch(secondDigit)
		{
			case 0: text = "Ten";
			break;
			case 1: text = "Eleven";
			break;
			case 2: text = "Twelve";
			break;
			case 3: text = "Thirteen";
			break;
			case 4: text = "Fourteen";
			break;
			case 5: text = "Fifteen";
			break;
			case 6: text = "Sixteen";
			break;
			case 7: text = "Seventeen";
			break;
			case 8: text = "Eighteen";
			break;
			case 9: text = "Nineteen";
		}
				
		return text;
	}
}
	
//brings in second digit from number of bottles remaining
//returns that digit in text form
string funcSecondDigit(int digit)
{
	string text;
	
	switch(digit)
	{
		case 0: text = "";
		break;
		case 1: text = "One";
		break;
		case 2:	text = "Two";
		break;
		case 3: text = "Three";
		break;
		case 4: text = "Four";
		break;
		case 5: text = "Five";
		break;
		case 6: text = "Six";
		break;
		case 7: text = "Seven";
		break;
		case 8: text = "Eight";
		break;
		case 9: text = "Nine";
	}
		
	return text;
}

[hamsterman]

Your code is quite long. You do the same thing many times. Here's how I did it:

#include <iostream>
#include <string>
using namespace std;

string number(int n);

int main(){
	for(int bottles = 99; bottles > 0; bottles--){
		cout << number(bottles) << (bottles == 1 ? " bottle" : " bottles") << " of beer on the wall\n";
		cout << number(bottles) << (bottles == 1 ? " bottle" : " bottles") << " of beer\n";
		cout << "Take one down, pass it around,\n";
		cout << number(bottles-1) << (bottles == 2 ? " bottle" : " bottles") << " of beer on the wall\n";
		cin.get();
	}
}

string num1[] = {"zero", "one", "two", "three", "four", "five", "six", "seven", "eight", "nine"};
string num2[] = {"ten", "eleven", "twelve", "thirteen", "fourteen", "fifteen", "sixteen", "seventeen", "eighteen", "nineteen"};
string num3[] = {"twenty", "thirty", "fourty", "fifty", "sixty", "seventy", "eighty", "ninety"};

string number(int n){
	string result;

	if(n < 10) result = num1[n];
	else if(n < 20) result = num2[n-10];
	else{
		int dig1 = n/10, dig2 = n%10;
		result = num3[dig1-2];
		if(dig2 != 0) result += "-" + num1[dig2];
	}

	result[0] = toupper(result[0]);
	return result;
}

[gers1978]

Thanks, the bit of the book I'm at has only so far talked about variables, ifs, switches, and functions, not arrays.

I'm not sure what most of your code does?

[Athar]

That explains all the duplicate code.
Make sure to read the chapter about arrays soon, they are fairly easy to understand and essential for serious programming.

[hamsterman]

I'm not sure what most of your code does?

I'd be glad to help, but you'll have to be more specific.

[gers1978]

Athar: I actually know a little bit about arrays myself, I just assumed that the project didn't want me/require me to use them since they hadn't been covered yet in the book

hamsterman: As best I can see, your code:

1) declares a function that returns a string and accepts an integer

2) Has a for loop that counts down from 99. I'm not sure what the ? symbol and colon (:) do in your cout lines

3) declares 3 arrays of strings, 1 for 0-9, one for 10, 11, 12 and the teens, one for 20-90

4) Your number function checks to see if remaining bottles is less than 10, if so outputs the element (bottles) from the first array, failing that uses array 2 (the else if statement)

5) I'm struggling also to understand the else statement at all, and I have no idea what the result[0] = toupper... line does.

Also, I fear my big problem with programming is: How the hell do you come up with these better ways of doing things? I feel like I can learn the syntax and rules of a language ok, and I can read through and follow other people's code THEN understand it, but how do you come up with the ideas/solutions to start with? That's what I fear I may never be able to grasp.

Thanks for all the help, I'm keen to learn.

[firedraco]

2) ? : -> see: http://www.cplusplus.com/doc/tutorial/operators/

5)

else{
    int dig1 = n/10, //int for the ones place, uses integer division (21/10 = 2.1 = 2) to find offset
        dig2 = n%10; //int for the tens place, uses modulus (21%10 = 1) to find offset
    result = num3[dig1-2]; //result is the tens place (dig1-2)
    //the -2 is the offset since arrays are 0 based AND we also only have 8 values
    if(dig2 != 0) result += "-" + num1[dig2]; //attach the ones place
    //if the ones place is zero, we don't attach anything
}

result[0] = toupper(result[0]); //this just makes the first character uppercase
//"mystr" -> "Mystr"
//note this will fail if result is an empty string but we are ok here 

And...

How the hell do you come up with these better ways of doing things?

Experience usually...you eventually can look at a problem and say "Oh, this is basically X said in a different way, so I'll do some sort of Y..." and then build from there.

[RyanCaywood]

Things become intuitive, and once you start to learn more about the details of the language, what you can do with it, etc, this kinda stuff will become second nature. You will just look at the code and ... well firedraco explained it pretty well.

Anyways... Just for fun, I tried to convert it to C#, since I'm learning it along with C/C++ and some others (C# is SOO easy to learn compared to C++, but C++ was my first lang too... I think that makes everything easier (with few exceptions)), though at times it makes it harder to code C++... But I need the skill in C# as its a very common language these days (I don't like .NET, but I like how easy it makes it to code... conflicting feelings there...).

Anyway, you'd be surprised how very little it took to convert it to C#. I only really had to change the std::cout functions to .NET's Console.WriteLine() function. As well as putting it in a class and namespace of it's own, and some minor differences with how variables are dealt with... But it only took me 5 minutes to get it working (would have been less if my computer was faster, it actually takes a second or even two or three sometimes to type in one character! it's mainly cause VS2010 is a bastard... Didn't have any problems with Eclipse). Anyway, the fact that it was nearly effortless to convert to C# could be either good, bad, or irrelevant to you, I really couldn't say for sure.

I'll post about what can be improved and my thoughts about the code, etc, tomorrow.

Really in all honesty, you're doing pretty well, for a complete newbie. Chapter 4 of the book you say? That's pretty good. I think I was at your level after about.... well I had messed with it a bit before but not seriously until a month or two ago... but it took me at least 2, possibly 3 weeks of intensive work with C++ to get to that level, then I kind of got a lot faster at learning stuff, once i knew the basics. I didn't take any classes or books by the way, just skimming over online tuts, reading the C++ reference, trial & error, trying to solve problems, etc.

My first suggestion would definitely be to try to minimize the redundancy of the code. e.g. not having the same things repeated several dozen times... 200 lines of code is quite excessive for what the program actually does.

Two hundred and ninety nine lines of code on the wall, two hundred and ninety nine lines of code....

My best advice:
Practice, Practice, Practice!

No book or class can teach you what good ol' practice, and trial & error can (though they can help quite a bit, and make it faster).