Code Improvement?

[CheesyBeefy]

Hi, just learning and experimenting away at C++ and I was wondering if you could suggest any ways I can make my code more efficient. Well here's my code. Beware, it is VERY simple and serves no real function, just an experiment to learn.

[code=cpp]#include "stdafx.h"
#include <iostream>

using namespace std;

int main()

{
//variables
int choice;
char restart;

//main code
start:
cout << "Enter choice 1, 2 or 3." << endl;
cin >> choice;

switch( choice )
{
//case 1
case 1:
cout << "You chose 1" << endl;
break;

//case 2
case 2:
cout << "You chose 2" << endl;
break;

//case 3
case 3:
cout << "You chose 3" << endl;
break;

//invalid choice
default:
cout << "That is an invalid choice. Please try again." << endl;
}

cout << "Would you like to restart? (Y / N): ";
cin >> restart;

if( restart == 'Y' )
{
cout << "\n\n\n";
goto start;
}

else if( restart == 'y' )
{
cout << "\n\n\n";
goto start;
}

else return 0;

}[/code]

All tips and suggestions are greatly appreciated!

[kryptonite]

I don't really know, something like this would be a little more efficient. You could effectively recode this completely, but being a short and pretty useless code there's not much to do. In a code like this I don't think using 'case' is the most efficient method, takes a ton more lones of code than the way I did it. Also, I never use loops with 'goto' and neither do most people. Duoas or someone will probably laugh at my code and re-write it perfect, but for now here you go lol

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

using namespace std;

int main()

{
	//variables
	int choice;
	char restart;

	//main code
	cout << "Enter choice 1, 2 or 3." << endl;
	cin >> choice;

if (choice>0 && choice<4) 
{ 
      cout << "You chose" << choice << endl;
} else {
      cout << "Invalid choice, please try again" << endl;
      main();
}

	cout << "Would you like to restart? (Y / N): ";
	cin >> restart;

	if( restart == 'Y' || restart == 'y' )
	{
		cout << "Restarting...\n\n\n";
		main();
	} else return 0;

[Zhuge]

I agree, and I hear that goto is considered bad practice.

Also I think that the switch/case is what you should use since I assume if this were an actual program you would be doing different things based on the user's choice.

As for the goto you could do what kryptonite suggested, but I would have just used a while loop like this:

bool cont;
do {
    // ....
    if(restart == 'Y' || restart == 'y') cont = false;
    else cont = true;
} while(!cont);
return 0;

[helios]

Actually, this is faster than if:

switch(choice){
	case 1:case 2:case 3:
		std::cout <<"..."<<choice<<std::endl;
		break;
	default:
		std::cout <<"..."<<std::endl;
}

I can't really remember the reason. Something about the jump depending on the value of the variable instead of doing comparisons. Of course, it has the disadvantage of having to list all the possible cases, so only use it for short lists.

[kryptonite]

**EDIT**

[kryptonite]

**EDIT**

[helios]

I never use using namespace, so I'm so used to typing std::cout that it's actually faster if I do. And the compiler doesn't complain if you use it with using namespace, so it's alright.

I was suggesting replacing the if with the switch. Calling main() is just stupid, no offense. goto is a lot (I'll emphasize this. A LOT) better than infinite recursion.

[Duthomhas]

Just to add to what helios said:

ISO C++ forbids you to call main().
In any case, it is a very dangerous thing to do. Even if nothing else goes wrong, you'll eventually exhaust the stack.

[kryptonite]

Yah, your right. Duoas do you reply to PM's? I honestly jumped straight from coding nothing to advanced programs and have never had a situation where I needed to loop something like main. I know a lot of stuff about memory functions and more advanced functions than I do about the basics - which can be good but obviously not good at the same time lol

[CheesyBeefy]

Well I changed some stuff up however a function I'm using isn't working. It's supposed to wait for the input number of seconds before running the next line of the program, however I get this error: 1>c:\documents and settings\brad\my documents\visual studio 2008\projects\switch\switch\switch.cpp(11) : fatal error C1903: unable to recover from previous error(s); stopping compilation However, I used this same function in another program and it worked fine. Here is my current code:
[code=cpp]#include "stdafx.h"
#include <iostream>

using namespace std;

//wait function
void Wait(int seconds)
{
clock_t endwait;
endwait = clock () + seconds * CLK_TCK ;
while (clock() < endwait) {}
}

int main( void )

{
//variables
int choice;
char restart;

//main code
start:
cout << "Enter choice 1, 2 or 3." << endl;
cin >> choice;

switch( choice )
{
//case 1
case 1:
cout << "You chose 1" << endl;
break;

//case 2
case 2:
cout << "You chose 2" << endl;
break;

//case 3
case 3:
cout << "You chose 3" << endl;
break;

//invalid choice
default:
cout << "That is an invalid choice. Please try again." << endl;
}

cout << "Would you like to restart? (Y / N): ";
cin >> restart;

if( restart == 'Y' || restart == 'y' )
{
cout << "\n\n\n";
goto restart;
}

else return 0;

restart:

int i = 0;

while( i > 2 )
{
cout << "Restarting .\n";
Wait( 1 );
cout << "Restarting ..\n";
Wait( 1 );
cout << "Restarting ...\n";
i = i + 1;
}

cout << "\n\n\n\n\n\n\n";
goto start;

}[/code]

[helios]

Is that the only error? Because that usually appears when there are more errors before.
CLK_TCK should be CLOCKS_PER_SEC.

[CheesyBeefy]

Oops, my problem was that I didn't include <time.h>. Silly me.

[Zhuge]

If you are coding in the C++ the "correct" way to include that file would be #include <ctime> .

(I say "correct" because, at least in VC++2008, all including <ctime> does is disable the warning and include <time.h>)