Sign in to follow this  

Are labels evil?

This topic is 4664 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

void saveHero()
{
	cout << "Tired, " << hero.getName() << " walks to Save Lake to drink some of it's refreshing water..." << endl;
	cout << "Save to Slot (1-3):";
	cin >> input;
	playCursorSound();
	
	if(input > 3 || input < 0)
		input = 3;

	cout << "Are you sure you want to save to slot " << input << "? (Y or N): ";
	cin >> charInput;

	if(charInput == 'N' || charInput == 'n')
		goto End;

	switch(input)
	{
		case 1:
			fout.open("hero1.txt");
			break;
		case 2:
			fout.open("hero2.txt");
			break;
		default:
			fout.open("hero3.txt");
			break;
	}
	if(fout.fail())
	{
		cout << "\nERROR: Could not open file!" << endl;
		goto End;
	}
	fout << "Name: " << hero.getName() << endl;
	fout << "Class: " << hero.getClass() << endl;
	fout << hero.getDesc() << endl;
	fout << "MaxHP: " << hero.getMaxHP() << endl;
	fout << "MaxMP: " << hero.getMaxMP() << endl;
	fout << "HP: " << hero.getHP() << endl;
	fout << "MP: " << hero.getMP() << endl;
	fout << "Str: " << hero.getStr() << endl;
	fout << "Def: " << hero.getDef() << endl;
	fout << "Int: " << hero.getInt() << endl;
	fout << "Dex: " << hero.getDex() << endl;
	fout << "Gold: " << hero.getGold() << endl;
	fout << "Helm: " << hero.getHelm() << endl;
	fout << "Armor: " << hero.getArmor() << endl;
	fout << "Weapon: " << hero.getWeapon() << endl;
	fout << "Extra: " << hero.getExtra() << endl;
	fout << "Items: 0 " << hero.getItemPotion() << " 1 "
			<< hero.getItemSuperPotion << " 2 " << hero.getItemElixer()
			<< " 3 " << hero.getItemSuperElixer()
			<< " 4 " << hero.getItemBeserkPotion()
			<< " 5 " << hero.getItemMagicGem() << endl;
	fout.close();

	cout << "The Legendary " << hero.getName() << " Saved." << endl;

	End:
	cout << endl;
}

In every book or discussion, they always say that labels are evil. Well, when looking at this code, I could not find any easier way of doing it rather than using a label. Since labels are evil, what do I do now?! =)

Share this post


Link to post
Share on other sites
if(fout.fail())
{
cout << "\nERROR: Could not open file!" << endl;
return;
}


if(fout.fail())
{
throw std::exception("ERROR: Could not open file!");
}


...

But no, goto aren't really evil. It's just that using one must be a deliberate decision, i.e. when the alternative to using one makes the code awkward, hard to understand...

Share this post


Link to post
Share on other sites
I can see only one valid reason for using gotos (e.g. labels) in C(++): breaking from nested loops across several levels. It's the most efficient way to achieve that. In most (if not any) other cases you can simply replace the construct with an if-else expression (or a break/continue in case of loops) without performance loss.
The problem with using gotos is not the construct itself, but improper/unnecessary or over-use of it.
The example you provided is not a good use of a goto. Aside from the fact that the condition is incorrect (e.g. any input other than 'n' leads to save while the text clearly states that only 'y' and 'n' are acceptable inputs[wink]), it can easily be replaced by

if (charInput != 'N' && charInput != 'n') {

// create file

if (fout.fail()) {
// notify failure
}

else {
// write data
}
}


Happy coding,
Pat.

Share this post


Link to post
Share on other sites
Quote:
Original post by darookie
Quote:
Original post by Sneftel
Why not just return?

Even better yet. *Turning of SFEP-advocate-mode*[wink]


Advocating single entry-point makes sense. Single exit-point does not.
Thankfully, C++ does not allow you to goto into a function.

Share this post


Link to post
Share on other sites
Quote:
Original post by Fruny
Quote:
Original post by darookie
Quote:
Original post by Sneftel
Why not just return?

Even better yet. *Turning of SFEP-advocate-mode*[wink]


Advocating single entry-point makes sense. Single exit-point does not.
Thankfully, C++ does not allow you to goto into a function.

I will not argue against that. Generally SFEP messes complex code up and leads to no good. For trivial functions I prefer it, though.

Share this post


Link to post
Share on other sites
You could also break up the function - this sort of thing can be an indication that you're trying to do too much:


void saveHero() {
int slot = promptForSlot();
if (slot && saveToSlot(slot)) {
cout << "The Legendary " << hero.getName() << " Saved." << endl;
}
cout << endl;
}

int promptForSlot() {
int slot;
char yesno;
cout << "Tired, " << hero.getName() << " walks to Save Lake to drink some of it's refreshing water..." << endl;
cout << "Save to Slot (1-3):";
cin >> slot;
playCursorSound();

if (slot > 3 || slot < 0) slot = 3;

cout << "Are you sure you want to save to slot " << slot << "? (Y or N): ";
cin >> yesno;

if (yesno == 'N' || yesno == 'n') slot = 0;
return slot;
}

bool saveToSlot(int slot) {
ifstream fout((stringstream("hero") << slot << ".txt").str().c_str());
bool fileFound = fout.good();
if(fileFound) {
// Do all the file writing here... BTW, you may want to look into better
// techniques for this
} else {
cout << "\nERROR: Could not open file!" << endl;
}
fout.close();
return fileFound;
}



P.S. I get the general impression that you are using member variables for things that ought to be more restricted in scope (i.e. locals). This is nearly as bad as using globals for the same purpose. It's confusing because it implies there's a reason for the extra scope when there isn't.

Share this post


Link to post
Share on other sites
SFEP might be a good in pure C, but in C++ it not going to save you from things like exceptions. On the other hand, if you wrap up things like pointers in objects that will clean things up in their destructor (hint: std::auto_ptr) you can return anywhere you want, be exception safe, and write less code, all the the same time.

Share this post


Link to post
Share on other sites
Quote:

if (charInput != 'N' && charInput != 'n') {

// create file

if (fout.fail()) {
// notify failure
}

else {
// write data
}
}


It would be safer if I did it the other way, since that way, it would override the file only if it is "y" or "Y".

Quote:

P.S. I get the general impression that you are using member variables for things that ought to be more restricted in scope (i.e. locals). This is nearly as bad as using globals for the same purpose. It's confusing because it implies there's a reason for the extra scope when there isn't.


You mean like the var input? I guess I could make them all local. -_-

Quote:

// Do all the file writing here... BTW, you may want to look into better
// techniques for this


This was the only way I knew how to do it. Any suggestions? XD

Looking back, I guess my code is really messy, since this is basically the first project that went over 2000 lines of code. =)

Share this post


Link to post
Share on other sites
If you want to know why people say gotos and labels are evil, download the original C source code of the mainframe version of Zork and try to figure out the program logic.

Share this post


Link to post
Share on other sites
Quote:
Original post by WiseElben
It would be safer if I did it the other way, since that way, it would override the file only if it is "y" or "Y".


Good call. :)

Quote:

You mean like the var input? I guess I could make them all local. -_-


Yeah. Member variables basically are intended to allow for communication between methods, and represent the persistent "state of the object". Where no such communication is necessary, the Prinicple of Least Priviledge demands that we not make it available, because
(a) we might make use of it accidentally, or with unexpected side effects:

class LoopProcessor {
int i;
// ...
void foo() {
for (i = 0; i < 10; i++) {
wibble();
bar();
}
}
void bar() {
for (i = 0; i < 5; i++) {
cout "Oops, foo is an infinite loop now" << endl;
}
}
}

b) It becomes harder to prove things about our code, because we have to check in more places to determine how and where the variable is being used. That slows down refactoring.

Quote:

This was the only way I knew how to do it. Any suggestions? XD


[google] serialization. "Serialize yourself" is a perfectly reasonable bit of behaviour to implement as an object method. Instead of asking the Hero to expose all the raw bits of information, you should make him do the work. One option for this is to make ostream::operator<< a friend of your class, and then overload it. Another is to provide a method like "std::string Hero::toString()". Also, ICBW about this, but I think you can overload operator()(ostream&), and it will work with operator<< without needing the "friendliness" of ostream. (That is, the stream will respond to the request "output this thing, which is of type 'can be called on an ostream'", by in turn asking the thing "execute using me".) If not, there is almost certainly a way to create that behaviour for the general case, and tuck it away in a library. (Heck, maybe it's in Boost or Loki or something, I dunno :) )

Quote:
Looking back, I guess my code is really messy, since this is basically the first project that went over 2000 lines of code. =)


Good stuff. Now is a good time to start learning what you can about cleaning code up, and see how much shorter it becomes once you apply what you've learned. :)

Share this post


Link to post
Share on other sites

This topic is 4664 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this