Before anyone thinks about flaming me, no, I don't use goto myself. This comment got me thinking about it.
Assembler (Z80) has various jump instructions, some conditional, followed by the memory location to jump to. These differed to a call instruction in that once you jumped, you couldn't return (the address jumped from wasn't pushed onto a stack, whereas the calling address was).
With this in mind, is it wrong to use the occasional goto in code, other than for aesthetic reasons?
I have to admit that i have used in on occasion, when the alternative was to re-write a loop and add a bunch of variables and if's. Not in a large program however.
It's just very ugly and makes it hard to read the code
It's not wrong to use it were it is appropriate.
My ideas:
I would say GOTO is a BadThing for 99% of the cases; most programmers could code well enough to never need something like this.
I have seen a GOTO in a severely-nested function, though, which seemed to do alright. It was inside, I think, 8-9 while() and for() loops, and the point of the GOTO was "Okay, the value is now true/false. Continue with the rest of the current function!" Otherwise, he would have had to test on EVERY loop whether "done == true" (or whatever), and that was too expensive when you're dealing with a loop that was close to O(2^10) or something like that.
So my answer is that you should avoid it when possible, but if you HAVE to use it, do it only for efficiency reasons like that above.
In high level languages the use of goto can almost always be replaced by some other, more elegant solution. The only place in C where I ever use goto is to break out of some large nested loop. With some other languages, you can use the syntax break label; to avoid using a goto in such a scenario as that. Even in C, it's quite possible that such a loop can be better written as a function call that returns in place of the goto.
Matthew: I hadn't heard of "break label" before... That would be a useful tool if I ever start nesting...
Both Java and D use break label;. PHP uses break level;, where level is the number of loops of which you want to break out.
Goto makes programs hard to read and analyze. The only use for goto is to break from nested cycles, yes, but most times you can prevent that by adding control variables to each of the loops.
Goto can also be used to implement exception-like logic in languages like C. I remember having seen file access routines which had lots of potential failure points. Instead of repeating error handling code for each point, the routines used goto, which made them easier to understand.
A goto is only bad because people get lazy. Most of the time a goto is not needed (mostly only needed in low-level). Without the goto statement c and c++ are turing complete. The danger is someone could use a goto and avoid deep thinking and proper organization of the code (create a proper state machine and code it cleanly!). This is when it becomes hard/impossible to read and debug.
My question is this: If you use a GOTO to break out of a function, what happens to all the local variables? Are they taken out of scope, or can they still be accessed?
Oh, and I think I should start doing this from now on:
1 | int main(int argc, char **argv) |
2 | { |
3 | string temp; |
4 | for (int i = 0; i < argc; i++) |
5 | { |
6 | temp = argv<i>; |
7 | if (temp == "--help") |
8 | { |
9 | goto HELP; |
10 | HELPDONE: |
11 | } |
12 | else if (temp == "--version") |
13 | { |
14 | goto VERSION; |
15 | VERSIONDONE: |
16 | } |
17 | else if (temp == "--screenResolution") |
18 | { |
19 | goto RESOL |
20 | RESOLDONE: |
21 | } |
22 | else if (...) |
23 | } |
24 | |
25 | goto END; |
26 | |
27 | HELP: |
28 | cout << "You asked for help..."; |
29 | goto HELPDONE; |
30 | VERSION: |
31 | cout << "You're asking for the version..."; |
32 | goto VERSIONDONE; |
33 | RESOL: |
34 | cout << "You're asking for the screen resolution..."; |
35 | goto RESOLDONE |
36 | |
37 | END: |
38 | return 0; |
39 | } |
EDIT: Fixed the labels per Simon Parzer's feedback. Thanks, Simon!
In C goto can't take you outside the scope of the current function.
Oh, and I think I should start doing this from now on:
Oh no! Don't ever do that! That's what the switch clause is for.
Why don't you try it?
You got it all wrong BTW...
You do labels and gotos in C like this:
ThisIsALabel:; //some code goto ThisIsALabel;
I don't know exactly what you posted but I think the syntax is wrong.
In my opinion you should use goto where you need it. As long as you don't plan to share your code with others.
Sometimes it even makes code simpler. I mean, I rather have a bunch of gotos and three labels than five nested loops. When I started with BASIC a long time ago I got used to GOTOs and I have no big problems with them now.
In C++ or modular C the goto statement doesn't make much sense anymore, though.
Hey TT, your code put a grin on my face.
Sometimes, when writing my flow control, its difficult to implement it or think of it in terms of the standard C/C++ flow control primitives of for/while/do/switch loops and function calls. Usually in such cases I'll write what I'm thinking of in terms of loops and function calls, but do most of the flow control with if...break; and if...continue; statements and a few variables declared for the sole purpose of acting as flags for the loops.
Occaisonally though, the code will become harder to maintain, and/or debug if I write it that way, so I'll use goto statements instead. That has only happened maybe 10-20 times in the last 30 thousand lines of C/C++ code I've written, and some of those cases could have been expressed as exceptions more naturally if I hadn't been trying to avoid some issue involved with exceptions in the relevant code.
Even more rarely, sometimes I'll write something where I have such a difficult time even concieving of the flow control necessary that expressing it in terms of anything other than the natural flow control primitives I concieved of it in (often gotos for something like that) would take a great deal of diagramming and analysis and just be asking for trouble, so I write it as I concieve of it. That has only happened maybe 2 or 3 times in the last 30 thousand lines of C/C++ code I've written. Perhaps I should spend the time and effort diagramming and analyzing such cases; perhaps it would improve my ability to think in terms of the usual flow control primitives. But I'm not too sure about that. Perhaps that would merely limit my thinking to things that can easily be expressed in C/C++ instead.
I have never used goto in C/C++, and the only time I've ever used it is with VB 6 for error handling. Because I never use it, I never think about it, and therefore when coding, my mind never even considers it.
Ladies and gentlemen, I give you: the computed goto in C.
The &&X syntax is a DJGPPism for : "the memory address of label X".
1 | #define AMC_GOTO_PCODE goto *pcode[*code_pointer] |
2 | |
3 | static const void * pcode[] = { |
4 | NULL, |
5 | &&AMCInt, // 1 |
6 | &&AMCRegGlobal,// 2 |
7 | &&AMCRegLocal, // 3 |
8 | &&AMCRnd, // 4 |
9 | &&AMCJump, // 5 |
10 | &&AMCDirect, // 6 |
11 | &&AMCExit, // 7 |
12 | &&AMCIfnot, // 8 |
13 | &&AMCIfjump, // 9 |
14 | &&AMCIfdirect, // 10 |
15 | &&AMCIfexit, // 11 |
16 | &&AMCEnd, // 12 |
17 | &&AMCAuto, // 13 |
18 | &&AMCStore, // 14 |
19 | &&AMCRecall, // 15 |
20 | &&AMCStackup, // 16 |
21 | &&AMCSpawn, // 17 |
22 | &&AMCPause, // 18 |
23 | &&AMCMove // 19 |
24 | }; |
25 | |
26 | (...) |
27 | // start of interpretation |
28 | AMC_GOTO_PCODE; |
29 | |
30 | (...) // some sample instructions : |
31 | AMCInt: // put an Integer in the register |
32 | code_pointer++; |
33 | AmEval(¤texp,code_pointer,*(code_pointer+1)); |
34 | code_pointer+=2; |
35 | AMC_GOTO_PCODE; |
36 | |
37 | AMCJump: // Jump |
38 | code_pointer++; |
39 | code_pointer= *code_pointer + BOB[Actor].programstart; |
40 | AMC_GOTO_PCODE; |
Compared to a switch() statement, this was way cooler IMHO, and the emulation time went from 3% to 2% (precision of this figure is low, though)
I've never used goto. I do think it is a bad thing. The more you use goto, the more your code differs from what is considered typical C code structure. As Matthew said, you can put the part of code in a function and have returns instead.
Johan, I would agree with that, unless there has to be a lot of data transfer.
Say, for example, that you have declared a bunch of data at the beginning of your function. Then, you want to test whether "a == b" and "c < d" or "e != f", etc. If you push all that data into a separate function just to do the test, it slows down the code quite a bit.
If, however, you break out of a very-nested group of loops with a GOTO, you can continue with all the data and variables you had declared/changed/etc. as if you had simply "break"-ed a bunch of times to get there...
Here's some code I have for decoding macroblocks from an MPEG-1 data stream:
1 | static void p_picture(APEG_LAYER *layer) |
2 | { |
3 | const int MBAmax = layer->mb_cols*layer->mb_rows; |
4 | int macroblock_type; |
5 | int coded_block_pattern; |
6 | int MBA, MBAinc; |
7 | int dc_dct_pred[3]; |
8 | int PMV[2]; |
9 | int bx, by; |
10 | unsigned int code; |
11 | |
12 | slice_start: |
13 | dc_dct_pred[0] = dc_dct_pred[1] = dc_dct_pred[2] = 0; |
14 | PMV[0] = PMV[1] = 0; |
15 | |
16 | code = apeg_start_code(layer); |
17 | if(code < SLICE_START_CODE_MIN || code > SLICE_START_CODE_MAX) |
18 | return; |
19 | apeg_flush_bits32(layer); |
20 | |
21 | slice_header(layer); |
22 | |
23 | bx = get_mba_inc(layer); |
24 | by = (code&255) - 1; |
25 | |
26 | MBA = by*layer->mb_cols + bx; |
27 | |
28 | bx <<= 4; |
29 | by <<= 4; |
30 | |
31 | block_start: |
32 | code = show_bits(layer, 6); |
33 | |
34 | if(code >= 8) |
35 | { |
36 | code >>= 3; |
37 | |
38 | apeg_flush_bits8(layer, PMBtab0[ code ].len); |
39 | macroblock_type = PMBtab0[ code ].val; |
40 | |
41 | dc_dct_pred[0] = dc_dct_pred[1] = dc_dct_pred[2] = 0; |
42 | |
43 | switch(macroblock_type & (MACROBLOCK_MOTION_FORWARD|MACROBLOCK_PATTERN)) |
44 | { |
45 | case MACROBLOCK_MOTION_FORWARD|MACROBLOCK_PATTERN: |
46 | apeg_motion_vector(layer,PMV,forward_code,full_forward_vector); |
47 | apeg_form_f_pred(layer, bx, by, PMV); |
48 | |
49 | break; |
50 | |
51 | case MACROBLOCK_PATTERN: |
52 | PMV[0] = PMV[1] = 0; |
53 | apeg_empty_pred(layer->forward_frame, layer->current_frame, bx, by, layer->coded_width); |
54 | |
55 | break; |
56 | |
57 | default: |
58 | apeg_motion_vector(layer,PMV,forward_code,full_forward_vector); |
59 | apeg_form_f_pred(layer, bx, by, PMV); |
60 | |
61 | goto next; |
62 | } |
63 | } |
64 | else |
65 | { |
66 | apeg_flush_bits8(layer, PMBtab1[ code ].len); |
67 | macroblock_type = PMBtab1[ code ].val; |
68 | |
69 | switch(macroblock_type & (MACROBLOCK_QUANT|MACROBLOCK_INTRA)) |
70 | { |
71 | case MACROBLOCK_QUANT|MACROBLOCK_INTRA: |
72 | layer->quantizer_scale = apeg_get_bits8(layer, 5); |
73 | // fall-through... |
74 | case MACROBLOCK_INTRA: |
75 | PMV[0] = PMV[1] = 0; |
76 | |
77 | apeg_decode_intra_blocks(layer, dc_dct_pred); |
78 | |
79 | apeg_fast_idct(apeg_block[0]); |
80 | apeg_fast_idct(apeg_block[1]); |
81 | apeg_fast_idct(apeg_block[2]); |
82 | apeg_fast_idct(apeg_block[3]); |
83 | apeg_fast_idct(apeg_block[4]); |
84 | apeg_fast_idct(apeg_block[5]); |
85 | |
86 | Move_Blocks(layer, bx, by); |
87 | |
88 | goto next; |
89 | |
90 | case 0: // Table 1 must have Intra and/or Quant flags set |
91 | goto slice_start; |
92 | } |
93 | |
94 | layer->quantizer_scale = apeg_get_bits8(layer, 5); |
95 | dc_dct_pred[0] = dc_dct_pred[1] = dc_dct_pred[2] = 0; |
96 | |
97 | if(macroblock_type & MACROBLOCK_MOTION_FORWARD) |
98 | { |
99 | apeg_motion_vector(layer,PMV,forward_code,full_forward_vector); |
100 | apeg_form_f_pred(layer, bx, by, PMV); |
101 | } |
102 | else |
103 | { |
104 | PMV[0] = PMV[1] = 0; |
105 | apeg_empty_pred(layer->forward_frame, layer->current_frame, bx, by, layer->coded_width); |
106 | } |
107 | } |
108 | |
109 | coded_block_pattern = get_block_pattern(layer); |
110 | |
111 | do_block(0); |
112 | do_block(1); |
113 | do_block(2); |
114 | do_block(3); |
115 | do_block(4); |
116 | do_block(5); |
117 | |
118 | next: |
119 | if(++MBA >= MBAmax) |
120 | return; |
121 | |
122 | if(show_bits(layer, 24) == 1) |
123 | goto slice_start; |
124 | |
125 | MBAinc = get_mba_inc(layer); |
126 | if(MBAinc != 0) |
127 | { |
128 | int i = MBAinc; |
129 | |
130 | dc_dct_pred[0] = dc_dct_pred[1] = dc_dct_pred[2] = 0; |
131 | PMV[0] = PMV[1] = 0; |
132 | |
133 | do { |
134 | if((bx += 16) == layer->coded_width) |
135 | { |
136 | bx = 0; |
137 | by += 16; |
138 | } |
139 | apeg_empty_pred(layer->forward_frame, layer->current_frame, bx, by, layer->coded_width); |
140 | } while(--i); |
141 | |
142 | MBA += MBAinc; |
143 | if(MBA >= MBAmax) |
144 | return; |
145 | } |
146 | |
147 | if((bx += 16) == layer->coded_width) |
148 | { |
149 | bx = 0; |
150 | by += 16; |
151 | } |
152 | |
153 | goto block_start; |
154 | } |
It's pretty clear to see the code flow, IMO. 'goto block_start', and it goes to decode the beginning of a block. 'goto slice_start', and it goes to start decode the beginning of a series of blocks. 'goto next', and it goes to find the next block of the slice.
Compare that with a double nested loop (adding two more indentations to a good portion of the code), extra if checks, ambiguous 'continue' and 'break' commands, and needing to trace close brackets to see where something loops back to..
PS: Whee, having PMBtab0[code] in my code breaks the syntax parser.
How bad is a goto statement?
goto jail;
We have managed to produce way over one million lines of C++ code with the only goto statements being in the various implementations of the quicksort algorithm. So I would advocate to not use goto, except when it makes algorithms easier to read, which is most often not the case. So, IF the algorithm is easier to read and understand using goto, then use it (but with great care!)...
When I was still learning how to program I would constantly use goto. Especially when I was first learning BASIC because the gosub command didn't make sense to me.
Learning C/C++ eventually taught me why goto was stupid for most situations.
But, I do still use it, though only in one very specific situation: To restart a procedure. My goto statements always return an executing procedure right back to where it started, or as close as is necessary for the purpose at hand, thus eliminating the need to have multiple nested do loops or recursion.
Goto is generally avoidable, but in the uncommon chance that it makes your code MORE readable than a do loop or if statement, then by all means, use it.
--- Kris Asick (Gemini)
--- http://www.pixelships.com
goto jail;
When I was coding in QBasic I used to have label for a part of code that printed some generic error message and exited the program. It was always called hell
I think Sun has a good reason to not even include the goto-statement in Java.
It is seldom needed, and I don't use them particularly often, but goto statements can occasionally improve both efficiency and clarity. Once in a rare while they are the best solution; when this is the case, it is nothing short of stupid to religiously avoid them. Whatever the best tool for the job is, use it.
Consider this code:
handle_objectdeletion: for(multiset<CObject*, SSortObjects>::iterator it = CObjectList::o().begin();it != CObjectList::o().end();it++) // loop through STL-multiset (could be any other container as well) if ((*it)->get_x() + (*it)->get_w() < CCamera::o().get_x()) // check, if some statement about the object is true and delete it { CObjectList::o().remove(it); // delete one object from the container goto handle_objectdeletion; // we must break from the loop as the iterators might be invalid now... }
Got any advice to make it better?
use break instead of goto and put it inside two loops, outer one loops until it has reached the end of the container.
Something like this (grossly simplified to make things clearer):
1 | bool needsDelete=false; |
2 | vector<Data> data; |
3 | vector<Data>::iterator iter; |
4 | while(1){ |
5 | for (iter=data.begin(); iter!=data.end(); iter++){ |
6 | if (iter.needs_to_be_deleted()){ |
7 | needsDelete=true; |
8 | // found something needing removal, break from for loop. |
9 | break; |
10 | } |
11 | } |
12 | if (needsDelete){ |
13 | iter.remove(); |
14 | needsDelete=false; |
15 | } else { |
16 | // looped through every variable, nothing needs deletion, breaks from while(1) |
17 | break; |
18 | } |
19 | } |
Code not tested but in theory it should work. There are about a million other ways of doing the same thing.
[edit]
Of course if the container is big then some kind of system should be used that would prevent the inner for loop to start over every time something gets deleted.
If you want to remove all the items that match the test:
multiset<CObject*, SSortObjects>::iterator it; it = CObjectList::o().begin(); while(it != CObjectList::o().end()) // loop through STL-multiset (could be any other container as well) { if ((*it)->get_x() + (*it)->get_w() < CCamera::o().get_x()) // check, if some statement about the object is true and delete it { CObjectList::o().remove(it); // delete one object from the container it = CObjectList::o().begin(); // restart the whole loop } it++; }
In this case, it's possible because "it = CObjectList::o().begin()" is a clean "restart", and if you delete the last element it matches the end-of-loop condition.
Ugh, learn the STL
for(multiset<CObject*, SSortObjects>::iterator it = CObjectList::o().begin();it != CObjectList::o().end();) // loop through STL-multiset (could be any other container as well) if ((*it)->get_x() + (*it)->get_w() < CCamera::o().get_x()) // check, if some statement about the object is true and delete it { it = CObjectList::o().erase(it); // delete one object from the container } else it++;
The docs I found say erase() returns void
My method of restarting is inefficient. Better to memorize a reference to the element to remove, then increase the iterator, then remove the to_be_removed element.
edit: anyway, multiset is probably a bad example, as it's one of the STL containers which does not invalidate iterators on remove.
The docs I found say erase() returns void
Your docs are probably outdated.
Didn't read through carefully, didn't notice that we're talking about multisets here. Sorry Audric, you're right about that.
To clarify, multisets are AssociativeContainers, and those have a void erase() (they don't need the return value anyway). The iterator erase() is part of Sequence (which is a list, vector, deque etc.). My comment about outdated docs stemmed from the fact that in the past, even Sequences had a void erase() in some compilers.
With this in mind, is it wrong to use the occasional goto in code, other than for aesthetic reasons?
Once you start down the dark path, forever will it dominate your destiny, consume you it will.
-- Yoda
Personnally, I use it's much wronger to write things like:
// (inside main, for example) if (some big problem in initialization) { // exception handling allegro_exit(); exit(0); } else { // a few hundred lines here.... } // ...continued here... for some reason. argh! }
To clarify, multisets are AssociativeContainers, and those have a void erase() (they don't need the return value anyway). The iterator erase() is part of Sequence (which is a list, vector, deque etc.). My comment about outdated docs stemmed from the fact that in the past, even Sequences had a void erase() in some compilers.
IIRC, erase() returns void on associative containers UNLESS you're using microsofts implementation and/or documentation : )
Audric's statement gave me a question:
void some_function_other_than_main() { ... if (error) exit(1); // <-- This line }
Does the "exit(1)" do about the same thing as the following code (barring the fact that "goto" was not supposed to leave the current function, or that "exit()" passes a value to be returned):
void some_function_other_than_main() { ... if (error) goto End_of_Main_Function; // <-- This line } int main() { ... :End_of_Main_Function return 0; }
So if you don't have a chance to clean up a bunch of your dynamically-allocated arrays before your program does a nose-dive, they will NEVER reach the destructors for your classes and/or cleanup() code?
So if you don't have a chance to clean up a bunch of your dynamically-allocated arrays before your program does a nose-dive, they will NEVER reach the destructors for your classes and/or cleanup() code?
All destructors on objects will be called with exit. They will not with abort, though.
All destructors on objects will be called with exit. They will not with abort, though.
But only if the dynamic array was created inside a class (which has a proper destructor)?
int main() { int *theArray; theArray = new int[400]; exit(1); };
So here, you would have a memory leak, or so I would imagine.
I would think a much better design would be to have exit() call a "cleanup" function of the programmer's choice:
1 | int clean_all_my_stuff(int wasErrorOnExit) |
2 | { |
3 | destroy_bitmap(double_buffer); |
4 | delete [] globalArray; |
5 | |
6 | return wasErrorOnExit; |
7 | } |
8 | |
9 | int main() |
10 | { |
11 | ... |
12 | if (error) |
13 | exit(clean_all_my_stuff(1)); |
14 | |
15 | ... |
16 | return (clean_all_my_stuff(0)); // <-- Passes "0" to signify all went well |
17 | } |
So if you don't have a chance to clean up a bunch of your dynamically-allocated arrays before your program does a nose-dive, they will NEVER reach the destructors for your classes and/or cleanup() code?
I never use exit. Errors that force program shutdown usually appear while setting up the program. I just create a function for initialization that returns some value. If it is an error it returns an error value (duh!). When I see that an error occured I simply call the function that is responsible for cleaning up and return from main afterwards.
very interesting subject
example:
1 | while(....) |
2 | { |
3 | |
4 | |
5 | INSTRUCTION; |
6 | |
7 | here: |
8 | if() |
9 | {} |
10 | else if () |
11 | {} |
12 | |
13 | else() |
14 | { |
15 | goto: here; |
16 | } |
17 | |
18 | |
19 | } |
How should I delete goto ? In above example - somebody must do something, he won't go further if he won't do something, and if I delete "goto" then instructions be become executed and I don't want it - have you solution ?
1 | while(....) |
2 | { |
3 | INSTRUCTION; |
4 | |
5 | do { |
6 | bool repeat = false; |
7 | if() |
8 | {} |
9 | else if () |
10 | {} |
11 | else() |
12 | { |
13 | repeat = true; |
14 | } |
15 | } while( repeat ); |
16 | } |
for example.
1 | while(....) |
2 | { |
3 | INSTRUCTION; |
4 | do |
5 | { |
6 | if() |
7 | { |
8 | // blah |
9 | } |
10 | else if() |
11 | { |
12 | // blah |
13 | } |
14 | else() |
15 | { |
16 | continue; |
17 | } |
18 | } while (FALSE); |
19 | } |
kosmitek: both Matthew and Jonatan are correct: you would use some sort of loop (while, for, etc.) until you specify that you should break out of it (or, you can specify that you will only run it once UNLESS some condition is met).
1 | bool breakInsideLoop = false; |
2 | while(...) |
3 | { |
4 | EXPRESSION; |
5 | |
6 | while (!breakInsideLoop) |
7 | { |
8 | ... |
9 | |
10 | if (...) |
11 | breakInsideLoop = true; |
12 | } |
13 | |
14 | ... // Now, outside of the "inside" loop |
15 | } |
Actually i've just been reading/writting some assembly and for some reason this version seems a lot nicer. I guess it depends on the audience and the style of programming and what mood your in.
here: if() {} else if () {} else() { goto: here; }
but matts' version is quite nice as well. The adding an extra variable is kind of a nasty hack.
This is making my head hurt!
I guess we can all just start learning low-level programming and use the goto: statement a lot more ... "Let's see... push aex onto, uh... fex... then pop foo off of bar... Whoops; that should have been fee from bak. Where's my debugger again..?"
Actually i've just been reading/writting some assembly and for some reason this version seems a lot nicer.
But it really is a loop—it's not a one time jump. I prefer the do/while construct because of that. Ideally there would be a slimmer syntax like:
do { // ... if (foo) continue; // ... }
Either way, the use of goto (or a similar loop construct) usually indicates that you should consider rethinking your approach altogether. I think the biggest problem with relying on goto for anything is that you might end up relying on it elsewhere out of laziness.
Winduhs search just found 41 gotos in Allegro source tree on this computer...
Most of Allegro's goto usage is for error handling. Given C's lack of exceptions, there's not always a good alternative.
I think the reasonable response is to say that GOTO is but one tool of many in a programmer's kit. A good programmer knows when and how often to implement the tools at his/her disposal; that is, it's there if you feel the situation warrants it.
I've used a handful of GOTO statements in the past, and I'm sure at some point in the future it will crop up once more. I've larger things to worry about
I think that the anti-goto rhetoric is largely a leftover from the bygone era when many programers learned to program in asm or basic where gotos and jmps were natural. People who started with C or higher level languages usually use higher level flow control primitives because, for most code, they're easier to write, maintain, and think in.
Most of Allegro's goto usage is for error handling. Given C's lack of exceptions, there's not always a good alternative.
That's what longjmp() is for.
(/me is mostly joking)
edit: first paragraph was edited for clarity
I try change this code as you say (it is also in ATTACHMENT) - but when I changed it wrong run - for example if I press "M" then pawn doesn't run BUT when I next press "->" then it goes 2 times !
1 | |
2 | #include<iostream> |
3 | #include<ctime> |
4 | #include <allegro.h> |
5 | using namespace std; |
6 | |
7 | |
8 | |
9 | |
10 | |
11 | class player |
12 | { |
13 | private: |
14 | int money; |
15 | int health; |
16 | int weapon; |
17 | int power; |
18 | |
19 | |
20 | public: |
21 | player(int a, int b, int c, int d) |
22 | { |
23 | money=a; |
24 | health=b; |
25 | weapon=c; |
26 | power=d; |
27 | } |
28 | |
29 | int change_money(int x) |
30 | { |
31 | money=money+x; |
32 | return money; |
33 | } |
34 | |
35 | int change_health(int x) |
36 | { |
37 | health=health+x; |
38 | return health; |
39 | } |
40 | |
41 | |
42 | int change_weapon(int x) |
43 | { |
44 | weapon=weapon+x; |
45 | return weapon; |
46 | } |
47 | |
48 | |
49 | int change_power(int x) |
50 | { |
51 | power=power+x; |
52 | return power; |
53 | } |
54 | |
55 | ~player() {} |
56 | |
57 | }; |
58 | |
59 | |
60 | |
61 | |
62 | |
63 | void start_game(BITMAP *board, BITMAP *pawn, BITMAP *buffer) |
64 | { |
65 | |
66 | int tab1[2][10]={ |
67 | {100,220,370,500,700,700,500,350,220,100}, |
68 | {100,100,100,100,100,340,340,340,340,340}, |
69 | }; |
70 | |
71 | int tab2[10]={0,1,2,3,4,5,6,7,8,9}; |
72 | |
73 | int *index=&tab2[0]; |
74 | int *zero=&tab2[0]; |
75 | int *nine=&tab2[9]; |
76 | int module; |
77 | int eyes; |
78 | srand (time(0)); |
79 | int i=21; |
80 | int y=1; |
81 | |
82 | player game(99, 99, 99, 99); |
83 | |
84 | while(!key[KEY_ESC]&&i!=0) |
85 | { |
86 | i--; |
87 | eyes = 1+rand()%3; |
88 | clear_to_color(buffer,makecol(255,255,255)); |
89 | blit(board,buffer,0,0,0,0,board->w,board->h); |
90 | blit(pawn,buffer,0,0,tab1[0][*index],tab1[1][*index],pawn->w,pawn->h); |
91 | textprintf_ex(buffer, font, 267, 580, makecol(0, 0, 0), -1, "You have: %d money, %d health, %d weapon and %d power.", game.change_money(y), game.change_money(y), game.change_weapon(y), game.change_power(y)); |
92 | textprintf_ex(buffer, font, 267, 600, makecol(0, 0, 0), -1, "You throw: %d, it left yo %d movements", eyes, i); |
93 | textprintf_ex(buffer, font, 267, 620, makecol(0, 0, 0), -1, "-> - left, <- - right"); |
94 | textprintf_ex(buffer, font, 267, 640, makecol(0, 0, 0), -1, "ESC - the end of the game"); |
95 | blit(buffer,screen,0,0,0,0,SCREEN_W,SCREEN_H); |
96 | |
97 | |
98 | |
99 | |
100 | direction: |
101 | readkey(); |
102 | if (key[KEY_LEFT]) |
103 | { |
104 | if(*index>=0&&*index<=4) |
105 | { |
106 | if ((*index-eyes)>=*zero) |
107 | { |
108 | index=index-eyes; |
109 | } |
110 | else |
111 | { |
112 | |
113 | index=nine-(eyes-*index-1); |
114 | } |
115 | |
116 | } |
117 | else if(*index>=5&&*index<=9) |
118 | |
119 | { |
120 | if ((*index+eyes)<=*nine) |
121 | { |
122 | index=index+eyes; |
123 | } |
124 | else |
125 | { |
126 | module=eyes-(*nine-*index); |
127 | if (module<0) |
128 | { |
129 | module=-module; |
130 | } |
131 | index=zero+module-1; |
132 | } |
133 | |
134 | } |
135 | } |
136 | |
137 | |
138 | |
139 | |
140 | else if (key[KEY_RIGHT]) |
141 | { |
142 | if(*index>=0&&*index<=4) |
143 | { |
144 | index=index+eyes; |
145 | |
146 | } |
147 | else |
148 | { |
149 | index=index-eyes; |
150 | } |
151 | |
152 | |
153 | } |
154 | |
155 | |
156 | |
157 | else |
158 | { |
159 | if (!key[KEY_ESC]) |
160 | { |
161 | goto direction; |
162 | } |
163 | |
164 | } |
165 | |
166 | |
167 | |
168 | } |
169 | |
170 | delete game; |
171 | |
172 | |
173 | } |
174 | |
175 | |
176 | |
177 | |
178 | |
179 | |
180 | int main() |
181 | { |
182 | BITMAP *board = NULL; |
183 | BITMAP *pawn = NULL; |
184 | BITMAP *buffer = NULL; |
185 | allegro_init(); |
186 | install_keyboard(); |
187 | set_color_depth(32); |
188 | set_gfx_mode(GFX_AUTODETECT,1024,768,0,0); |
189 | buffer=create_bitmap(SCREEN_W,SCREEN_H); |
190 | |
191 | |
192 | board = load_bitmap("board.bmp",NULL); |
193 | pawn = load_bitmap("pawn.bmp",NULL); |
194 | |
195 | start_game(board, pawn, buffer); |
196 | |
197 | |
198 | destroy_bitmap(board); |
199 | destroy_bitmap(pawn); |
200 | destroy_bitmap(buffer); |
201 | allegro_exit(); |
202 | return 0; |
203 | } |
204 | END_OF_MAIN() |
after change with following code my program wrong run:
1 | |
2 | while(....) |
3 | { |
4 | INSTRUCTION; |
5 | |
6 | do { |
7 | bool repeat = false; |
8 | if() |
9 | {} |
10 | else if () |
11 | {} |
12 | else() |
13 | { |
14 | repeat = true; |
15 | } |
16 | } while( repeat ); |
17 | } |
1 | while(....) |
2 | { |
3 | INSTRUCTION; |
4 | do |
5 | { |
6 | if() |
7 | { |
8 | // blah |
9 | } |
10 | else if() |
11 | { |
12 | // blah |
13 | } |
14 | else() |
15 | { |
16 | continue; |
17 | } |
18 | } while (FALSE); |
19 | } |
also wrong run
1 | bool breakInsideLoop = false; |
2 | while(...) |
3 | { |
4 | EXPRESSION; |
5 | |
6 | while (!breakInsideLoop) |
7 | { |
8 | ... |
9 | |
10 | if (...) |
11 | breakInsideLoop = true; |
12 | } |
13 | |
14 | ... // Now, outside of the "inside" loop |
15 | } |
is also wrong run
If you're using the exact code that you've just put in those last three examples, then no, that won't work. That's just skeleton code to show you examples of how a loop can be done.
You want something like this:
1 | int done = 0; |
2 | |
3 | while (!done) |
4 | { |
5 | |
6 | if (key[KEY_LEFT]) |
7 | { |
8 | if(*index>=0&&*index<=4) |
9 | { |
10 | if ((*index-eyes)>=*zero) |
11 | { |
12 | index=index-eyes; |
13 | } |
14 | else |
15 | { |
16 | |
17 | index=nine-(eyes-*index-1); |
18 | } |
19 | |
20 | } |
21 | |
22 | else if(*index>=5&&*index<=9) |
23 | { |
24 | if ((*index+eyes)<=*nine) |
25 | { |
26 | index=index+eyes; |
27 | } |
28 | else |
29 | { |
30 | module=eyes-(*nine-*index); |
31 | if (module<0) |
32 | { |
33 | module=-module; |
34 | } |
35 | index=zero+module-1; |
36 | } |
37 | |
38 | } |
39 | |
40 | } |
41 | |
42 | else if (key[KEY_RIGHT]) |
43 | { |
44 | if(*index>=0&&*index<=4) |
45 | { |
46 | index=index+eyes; |
47 | |
48 | } |
49 | else |
50 | { |
51 | index=index-eyes; |
52 | } |
53 | |
54 | } |
55 | |
56 | else if (key[KEY_ESC]) done = 1; |
57 | |
58 | } |
LennyLen your code also is wrong Pawn don't want go and it is reason why I use "goto" here, every other method is wrong.
I didn't notice you were counting down i to give the player 20 moves.
Here's a version that works exactly like the executable you included. I've restructured the program completely though, and changed parts that could be made simpler.
You were also using the key[] array incorrectly, which is why you were getting multiple rolls each time you pressed a key.
main.cpp:
1 | #include <iostream> |
2 | #include <ctime> |
3 | #include <allegro.h> |
4 | #include "player.h" |
5 | |
6 | |
7 | using namespace std; |
8 | |
9 | |
10 | // Global variables |
11 | // lots of them as I was too lazy to restructure the program properly |
12 | BITMAP *board = NULL; |
13 | BITMAP *pawn = NULL; |
14 | BITMAP *buffer = NULL; |
15 | int module, eyes, y = 1, index = 0, moves = 20; |
16 | player game(99, 99, 99, 99); |
17 | |
18 | |
19 | // Function Declarations |
20 | void initiate(); |
21 | void shutdown(); |
22 | void run_game(); |
23 | void display_screen(); |
24 | |
25 | |
26 | // int main() |
27 | int main() |
28 | { |
29 | initiate(); |
30 | run_game(); |
31 | shutdown(); |
32 | return 0; |
33 | } |
34 | END_OF_MAIN() |
35 | |
36 | |
37 | // void initialize() - gets Allegro set up |
38 | void initiate() |
39 | { |
40 | allegro_init(); |
41 | install_keyboard(); |
42 | |
43 | set_color_depth(32); |
44 | if (set_gfx_mode(GFX_AUTODETECT_WINDOWED,1024,768,0,0) != 0) |
45 | { |
46 | allegro_message("Error with gfx mode!"); |
47 | exit(EXIT_FAILURE); |
48 | } |
49 | |
50 | buffer=create_bitmap(SCREEN_W,SCREEN_H); |
51 | |
52 | board = load_bitmap("board.bmp",NULL); |
53 | if (!board) |
54 | { |
55 | allegro_message("Error loading board.bmp!"); |
56 | exit(EXIT_FAILURE); |
57 | } |
58 | |
59 | pawn = load_bitmap("pawn.bmp",NULL); |
60 | if (!pawn) |
61 | { |
62 | allegro_message("Error loading pawn.bmp!"); |
63 | exit(EXIT_FAILURE); |
64 | } |
65 | } |
66 | |
67 | |
68 | // void shutdown() - frees memory and exits Allegro |
69 | void shutdown() |
70 | { |
71 | destroy_bitmap(board); |
72 | destroy_bitmap(pawn); |
73 | destroy_bitmap(buffer); |
74 | allegro_exit(); |
75 | } |
76 | |
77 | |
78 | // void run_game() - does main game loop |
79 | void run_game() |
80 | { |
81 | |
82 | srand (time(0)); |
83 | int press; |
84 | |
85 | while (moves > 0) |
86 | { |
87 | eyes = 1 + rand() % 3; |
88 | display_screen(); |
89 | |
90 | press = readkey(); |
91 | |
92 | if ( (press >> 8) == KEY_LEFT) |
93 | { |
94 | if ( (index >= 0) && (index <= 4) ) |
95 | { |
96 | if ( (index - eyes) >= 0) |
97 | index = index - eyes; |
98 | else |
99 | index = 9 - (eyes - index - 1); |
100 | } |
101 | else if ( (index >= 5) && (index <= 9) ) |
102 | { |
103 | if ( (index + eyes) <= 9) |
104 | index = index + eyes; |
105 | else |
106 | { |
107 | module = eyes - (9 - index); |
108 | if (module < 0) |
109 | module =- module; |
110 | index = 0 + module - 1; |
111 | } |
112 | |
113 | } |
114 | moves--; |
115 | } |
116 | |
117 | if ( (press >> 8) == KEY_RIGHT) |
118 | { |
119 | if ( (index >= 0) && (index <= 4) ) |
120 | index = index + eyes; |
121 | else |
122 | index = index - eyes; |
123 | moves--; |
124 | } |
125 | |
126 | if ( (press >> 8) == KEY_ESC) break; |
127 | |
128 | } |
129 | |
130 | } |
131 | |
132 | |
133 | // void display_screen() - updates and displays the buffer to the screen |
134 | void display_screen() |
135 | { |
136 | int tab1[2][10]={ |
137 | {100, 220, 370, 500, 700, 700, 500, 350, 220, 100}, |
138 | {100, 100, 100, 100, 100, 340, 340, 340, 340, 340}, |
139 | }; |
140 | |
141 | clear_to_color(buffer, makecol(255, 255, 255)); |
142 | blit(board, buffer, 0, 0, 0, 0, board->w, board->h); |
143 | blit(pawn, buffer, 0, 0, tab1[0][index], tab1[1][index], pawn->w, pawn->h); |
144 | textprintf_ex(buffer, font, 267, 580, makecol(0, 0, 0), -1, "You have: %d money, %d health, %d weapon and %d power.", game.change_money(y), game.change_money(y), game.change_weapon(y), game.change_power(y)); |
145 | textprintf_ex(buffer, font, 267, 600, makecol(0, 0, 0), -1, "You throw: %d, it left you %d movements", eyes, moves); |
146 | textprintf_ex(buffer, font, 267, 620, makecol(0, 0, 0), -1, "-> - left, <- - right"); |
147 | textprintf_ex(buffer, font, 267, 640, makecol(0, 0, 0), -1, "ESC - the end of the game"); |
148 | blit(buffer, screen, 0, 0, 0, 0, SCREEN_W, SCREEN_H); |
149 | } |
player.h:
1 | #ifndef PLAYER_H |
2 | #define PLAYER_H |
3 | |
4 | class player |
5 | { |
6 | private: |
7 | int money; |
8 | int health; |
9 | int weapon; |
10 | int power; |
11 | |
12 | public: |
13 | player(int a, int b, int c, int d); |
14 | int change_money(int x); |
15 | int change_health(int x); |
16 | int change_weapon(int x); |
17 | int change_power(int x); |
18 | |
19 | }; |
20 | |
21 | #endif // PLAYER_H |
player.cpp:
1 | #include "player.h" |
2 | |
3 | player::player(int a, int b, int c, int d) |
4 | { |
5 | money = a; |
6 | health = b; |
7 | weapon = c; |
8 | power = d; |
9 | } |
10 | |
11 | int player::change_money(int x) |
12 | { |
13 | money = money + x; |
14 | return money; |
15 | } |
16 | |
17 | int player::change_health(int x) |
18 | { |
19 | health = health + x; |
20 | return health; |
21 | } |
22 | |
23 | int player::change_weapon(int x) |
24 | { |
25 | weapon = weapon + x; |
26 | return weapon; |
27 | } |
28 | |
29 | |
30 | int player::change_power(int x) |
31 | { |
32 | power = power + x; |
33 | return power; |
34 | } |
Thanks LennyLen, but it is also wrong because:
please start this game, please press "m" or other letter on keyboard 5 or more times and you see that information: "You throw: X " - and that X(number of eyes) it will be change - and X(number of eyes) should change ONLY when somebody press "->" or "<-", when somebody press something else X(number of eyes) shouldn't change - it is a problem which can be solved only when wy use "goto" - that I think.
This new run_game() function "fixes" the problem of it rolling when other keys are pressed. I'll leave it up to you to figure out how to only display information when M is pressed.
1 | void run_game() |
2 | { |
3 | |
4 | srand (time(0)); |
5 | int press; |
6 | |
7 | display_screen(); |
8 | |
9 | while (moves > 0) |
10 | { |
11 | eyes = 1 + rand() % 3; |
12 | |
13 | press = readkey(); |
14 | |
15 | if ( (press >> 8) == KEY_LEFT) |
16 | { |
17 | if ( (index >= 0) && (index <= 4) ) |
18 | { |
19 | if ( (index - eyes) >= 0) |
20 | index = index - eyes; |
21 | else |
22 | index = 9 - (eyes - index - 1); |
23 | } |
24 | else if ( (index >= 5) && (index <= 9) ) |
25 | { |
26 | if ( (index + eyes) <= 9) |
27 | index = index + eyes; |
28 | else |
29 | { |
30 | module = eyes - (9 - index); |
31 | if (module < 0) |
32 | module =- module; |
33 | index = 0 + module - 1; |
34 | } |
35 | |
36 | } |
37 | moves--; |
38 | display_screen(); |
39 | } |
40 | |
41 | if ( (press >> 8) == KEY_RIGHT) |
42 | { |
43 | if ( (index >= 0) && (index <= 4) ) |
44 | index = index + eyes; |
45 | else |
46 | index = index - eyes; |
47 | moves--; |
48 | display_screen(); |
49 | } |
50 | |
51 | if ( (press >> 8) == KEY_ESC) break; |
52 | |
53 | } |
54 | |
55 | } |
I would think a much better design would be to have exit() call a "cleanup" function of the programmer's choice:
Can't you use atexit() for that?
WOOOW !!!!! Run very good - I must seeing through very attentively - thank you LennyLen
--------------------------
ups - pawn after throw bone doesn't go after right fields - I tried change it but only when I use "goto" was okay
Here's a version I rewrote that extends the player class so that it now stores what tile the player is on, as well as the result of the latest roll. I also added a method to reroll the dice. The program now relies less on global variables, and is a little more object oriented.
I don't normally use C++, so there may be more efficient ways of doing things than I what I do.
By the way, I've left the logic of your game untouched, as I'm not 100% certain what you're trying to do. I'm curious as to why there's a lot more code for when the player presses left then for when they press right.
main.cpp:
player.h:
player.cpp:
LennyLen somewhere is little mistake - pawn after throw bone doesn't go after right fields - I tried change it but only when I use "goto" was okay
LennyLen somewhere is little mistake - pawn after throw bone doesn't go after right fields - I tried change it but only when I use "goto" was okay
The program I wrote behaves exactly as the one you wrote, so if there's a problem, it's in your logic. I've attached both your original program (I recompiled it to use the dynamic library so it was smaller) and the version I wrote so you can see for yourself that they behave the same.
Gee...
This sounds almost like the program I provided earlier to you, kosmitek, before you added the "player" class. Can't for the LIFE of me figure out why that code couldn't be used... It was re-written to be very understandable, use the exact same keys, handle the exact same logic...
Why not just extend what was already written for you and use that? It's not as if the code doesn't compile...
LennyLen, I don't know if you saw that earlier post or not (in the other thread, no less), but you may be able to use it. If not... shrug.
TeamTerradactyl yes, but I didn't use your earlier program because he also was wrong
In my program when I throw for example 3, pawn go 3 fields and in yours programs pawn is strange going - somewhere in your programs is little mistake, I must use "goto" in your program and only then pawn going right.
OK... you guys are posting about how GOTO is ugly and hard to read, and then post this as some sort of counter-example:
for(multiset<CObject*, SSortObjects>::iterator it = CObjectList::o().begin();it != CObjectList::o().end();) // loop through STL-multiset (could be any other container as well) if ((*it)->get_x() + (*it)->get_w() < CCamera::o().get_x()) // check, if some statement about the object is true and delete it { it = CObjectList::o().erase(it); // delete one object from the container } else it++;
I mostly use GOTOs for function-level error handling. Setting a flag, checking it in multiple points to break a loop/nest is slower and less efficient than a goto.
You guys are posting about how GOTO is ugly and hard to read, and then post this as some sort of counter-example:
Indeed. That code should be:
typedef std::multiset<CObject*, SSortObjects> myset_t; bool condition(myset_t::value_type &v) { return (v.get_x() + v.get_w() < CCamera::o().get_x()); } myset_t &myset = CObjectList::o(); myset.erase(std::remove_if(myset.begin(), myset.end(), condition), myset.end());
Somewhat cleaner now, although it's artificially lengthened by the additional typedefs and statement splittings.
I think in my program must be "goto", without "goto" my program isn't run good.
LENNY LEN - YOUR IDEA WITH 3 files: main.cpp; player.h; player.cpp, one table; and globall variables ARE WONDERFUL - WITHOUT THAT I MUSTED (MUST IN THE PAST ?) WRITE EVERYTHING IN ONE BIG FUNCTION AND NOW I HAVE MANY LITTLE FUNCTIONS - THANK YOU - THIS IDEA IS VERYYYYY GOOD - THANK YOU LENNY LEN FOR YOUR IDEA - I USE YOUR IDEA (but I had use "goto") AND I ADD GLOBAL TABLE - ONLY ONE TABLE !!! - THANKS YOU !!!!!!!!! YOU ARE WONDERFUL - THANK YOU !!!!!
MUST IN THE PAST ?
had to
Example: Because I forgot to turn off capslock I had to slap myself in the face several times.
LennyLen, I don't know if you saw that earlier post or not (in the other thread, no less), but you may be able to use it. If not... shrug.
Yeah I saw it. Most of my code was actually from when I rewrote the program last time kosmitek posted about it a few weeks ago.
TeamTerradactyl yes, but I didn't use your earlier program because he also was wrong
In my program when I throw for example 3, pawn go 3 fields and in yours programs pawn is strange going
The real problem was that you didn't explain very well what you were trying to do, and it's hard to tell from your code exactly what you are doing.
Now that I'm pretty certain that I know what you're trying to do, I've changed the logic to reflect this. I've attached the working code (the changes were to the run_game() function) and an executable. There's still no need for goto.
edit:
Don't rely too much on global variables. They certainly have their uses, but if you have too many of them, it makes the program harder to follow and less modular. There's usually a more elegant solution.
I think gotos can add much needed clarity to complex operations and I honestly think they are the shit. goto Failure is about as clear as you can get. There are a number of other completely valid and wonderfully clear uses of gotos.
The ability for a language feature to be misused does not render the feature obsolete.
now it runs ideal WITHOUT "GOTO" - LENNY LEN YOU ARE GENIUS !!!!!!
Your shift key is sticking again.
Thanks for the pretty code Bob, i didn't know you could handle it that way!
What i previously did:
//fl...ake is a vector iterator of the same type as snow //snow is a vector for(fl = snow.begin(); fl!=snow.end(); fl++){ if(fl->dead()){ snow.erase(fl); fl--; } }
Is this wrong or bug prone?
Is this wrong or bug prone?
Yes, fl is invalid when it is erased: you can't increment or decrement. In practice, however, it will work for vectors so long as you are not erasing the final element.
for (fl = snow.begin(); fl!=snow.end(); fl++) { if (fl->dead()) { snow.erase(fl); fl--; // <-- I think this is invalid } }
If I'm not mistaken, the vector iterator only has the ++ operator defined; not the --, so you could not decrement your current position...
//fl...ake is a vector iterator of the same type as snow
//snow is a vector
for(fl = snow.begin(); fl!=snow.end(); fl++){
if(fl->dead()){
snow.erase(fl);
fl--;
}
}
Is this wrong or bug prone?
Yes. This is what you want to do.
//fl...ake is a vector iterator of the same type as snow //snow is a vector for(fl = snow.begin(); fl!=snow.end(); ){ if(fl->dead()){ fl = snow.erase(fl); } else fl++; }
How about this?
snow.erase(std::remove_if(snow.begin(), snow.end(), std::mem_fun(&snow_type::dead)), snow.end());
(Untested code)
Cool , I'm totally gonna use that.
Thanks for the head-ups!