C++ Programmers: it is bad practice to write code like this

N

Norrin

Veteran XX
So I inherited a big project and I spent a little time understanding the following snippet. I post it here cause I'm bored and as an example of how not to code.

Code: 
next_try:
if( nPingCallCnt < PING_MAX_AMOUNT && IPList.GetCount())
{
    Sleep(PING_REPLAY_TIMEOUT);
    nPingCallCnt++;
}
else
{
    pCliPage->SetPingThreadStatus(PINGTHREADSTATUS_READY);
    return 0;
}

for(i=0; i<IPList.GetCount(); i++)
{
    // do stuff
}

RemoveIfEmptyIP(&IPList,&AppList,&PortList);
goto next_try;

pCliPage->SetPingThreadStatus(PINGTHREADSTATUS_READY);
return 0;

Notice the especially nice unreachable code at the end. So I change it to this:

Code: 
// Wait for 3 seconds, or until everyone we pinged has replied
while (nPingCallCnt < PING_MAX_AMOUNT && IPList.GetCount())
{
    Sleep (PING_REPLY_TIMEOUT);	// wait 1 second
    nPingCallCnt++;

    // Check for responses
    for (i = 0; i < IPList.GetCount(); i++)
    {
        // do stuff
    }

    // Remove anyone who replied
    RemoveIfEmptyIP(&IPList,&AppList,&PortList);
}

pCliPage->SetPingThreadStatus(PINGTHREADSTATUS_READY);
return 0;

The guy who wrote the original is a professional programmer from Poland. All of his code is like this. I've never seen so many freaking goto's in C++ code. It's a nightmare.
 
I hate putting the first curly brace on another line (I usually put a space then the brace), I don't know why. I guess I think it looks better (and I can tell the difference between blocks easily.)
 
Norrin said:
So I inherited a big project and I spent a little time understanding the following snippet. I post it here cause I'm bored and as an example of how not to code.

Code: 
next_try:
if( nPingCallCnt < PING_MAX_AMOUNT && IPList.GetCount())
{
    Sleep(PING_REPLAY_TIMEOUT);
    nPingCallCnt++;
}
else
{
    pCliPage->SetPingThreadStatus(PINGTHREADSTATUS_READY);
    return 0;
}

for(i=0; i<IPList.GetCount(); i++)
{
    // do stuff
}

RemoveIfEmptyIP(&IPList,&AppList,&PortList);
goto next_try;

pCliPage->SetPingThreadStatus(PINGTHREADSTATUS_READY);
return 0;

Notice the especially nice unreachable code at the end. So I change it to this:

Code: 
// Wait for 3 seconds, or until everyone we pinged has replied
while (nPingCallCnt < PING_MAX_AMOUNT && IPList.GetCount())
{
    Sleep (PING_REPLY_TIMEOUT);	// wait 1 second
    nPingCallCnt++;

    // Check for responses
    for (i = 0; i < IPList.GetCount(); i++)
    {
        // do stuff
    }

    // Remove anyone who replied
    RemoveIfEmptyIP(&IPList,&AppList,&PortList);
}

pCliPage->SetPingThreadStatus(PINGTHREADSTATUS_READY);
return 0;

The guy who wrote the original is a professional programmer from Poland. All of his code is like this. I've never seen so many freaking goto's in C++ code. It's a nightmare.
Click to expand...
the only problem i see is the 2 lines after the goto. If I'm reading it correctly, they will never get executed. Other then that, it's fine. Just a different coding style :shrug:
 
This is very bad practice, gotos should never be used except when you're testing and it doesn't matter. But it shouldn't go into the final product.

This is how my school puts it:

There are several statements in C++ that can alter the normal flow of execution through a program's statements. They are return, continue, break, and goto.

* return in any other position than at the end of a function is strongly discouraged.
* continue is strongly discouraged.
* break, except at the end of a case, is strongly discouraged
* goto should never be used.
Click to expand...
 
Last edited:
You like goto's and short circuit returns and no comments and complexity when a simple while suffices?

K. Remind me never to hire you.
 
Stinkfist said:
the only problem i see is the 2 lines after the goto. If I'm reading it correctly, they will never get executed. Other then that, it's fine. Just a different coding style :shrug:
Click to expand...

Using goto should have had him shot in the face in the first place.
 
Norrin said:
You like goto's and short circuit returns and no comments and complexity when a simple while suffices?

K. Remind me never to hire you.
Click to expand...

read his post, he inherited a project with this code, he didn't write it, you're making a fool of yourself.
 
Norrin said:
You like goto's and short circuit returns and no comments and complexity when a simple while suffices?

K. Remind me never to hire you.
Click to expand...
hey dumbass, i didn't say i liked it. I said i saw nothing WRONG with it. read -> comprehend.
 
Yeah, I remember back in high school we were not supposed to use GOTOs. Coming from BASIC as my first language, I was like WTF how am I ever going to get by without GOTOs. Then I realized the beauty of loops.
 
pshaaw! I code like this...

Code: 
while(nPingCallCnt<PING_MAX_AMOUNT&&IPList.GetCount()){Sleep
(PING_REPLY_TIMEOUT);nPingCallCnt++;for(i=0;i<IPList.GetCount();
i++){//dostuff}RemoveIfEmptyIP(&IPList,&AppList,&PortList);}
pCliPage->SetPingThreadStatus(PINGTHREADSTATUS_READY);return 0;

And comment my code? Fuck that! If the fuckers need to modify my code they shouldn't have fired my ass!

:bigthumb:
 
nspectre, then you'll love this

Code: 
[]include <stdio.h>

int main(int t, int _, char* a)
{
 return!0<t?t<3?main(-79,-13,a+main(-87,1-_,main(-86,0,a+1)+a)):
 1,t<_?main(t+1,_,a):3,main(-94,-27+t,a)&&t==2?_<13?
 main(2,_+1,"%s %d %d\n"):9:16:t<0?t<-72?main(_,t,
 "@n'+,#'/*{}w+/w#cdnr/+,{}r/*de}+,/*{*+,/w{%+,/w#q#n+,/#{l+,/n{n+,/+#n+,/#\
;#q#n+,/+k#;*+,/'r :'d*'3,}{w+K w'K:'+}e#';dq#'l\
 q#'+d'K#!/+k#;q#'r}eKK#}w'r}eKK{nl]'/#;#q#n'){)#}w'){){nl]'/+#n';d}rw' i;#\
 ){nl]!/n{n#'; r{#w'r nc{nl]'/#{l,+'K {rw' iK{;[{nl]'/w#q#n'wk nw'\
 iwk{KK{nl]!/w{%'l##w#' i; :{nl]'/*{q#'ld;r'}{nlwb!/*de}'c\
 ;;{nl'-{}rw]'/+,}##'*}#nc,',#nw]'/+kd'+e}+;#'rdq#w! nr'/ ') }+}{rl#'{n' ')#\
 }'+}##(!!/")
 :t<-50?_==*a?putchar(31[a]):main(-65,_,a+1):main((*a=='/')+t,_,a+1)
 :0<t?main(2,2,"%s"):*a=='/'||main(0,main(-61,*a,
 "!ek;dc i@bK'(q)-[w]*%n+r3#l,{}:\nuwloca-O;m .vpbks,fxntdCeghiry"),a+1);
}

believe it or not, that prints out all the verses of "The Twelve Days of Christmas".

:)

edit: forum mangled the include line.
 
Norrin said:
nspectre, then you'll love this

Code: 
[]include <stdio.h>

int main(int t, int _, char* a)
{
 return!0<t?t<3?main(-79,-13,a+main(-87,1-_,main(-86,0,a+1)+a)):
 1,t<_?main(t+1,_,a):3,main(-94,-27+t,a)&&t==2?_<13?
 main(2,_+1,"%s %d %d\n"):9:16:t<0?t<-72?main(_,t,
 "@n'+,#'/*{}w+/w#cdnr/+,{}r/*de}+,/*{*+,/w{%+,/w#q#n+,/#{l+,/n{n+,/+#n+,/#\
;#q#n+,/+k#;*+,/'r :'d*'3,}{w+K w'K:'+}e#';dq#'l\
 q#'+d'K#!/+k#;q#'r}eKK#}w'r}eKK{nl]'/#;#q#n'){)#}w'){){nl]'/+#n';d}rw' i;#\
 ){nl]!/n{n#'; r{#w'r nc{nl]'/#{l,+'K {rw' iK{;[{nl]'/w#q#n'wk nw'\
 iwk{KK{nl]!/w{%'l##w#' i; :{nl]'/*{q#'ld;r'}{nlwb!/*de}'c\
 ;;{nl'-{}rw]'/+,}##'*}#nc,',#nw]'/+kd'+e}+;#'rdq#w! nr'/ ') }+}{rl#'{n' ')#\
 }'+}##(!!/")
 :t<-50?_==*a?putchar(31[a]):main(-65,_,a+1):main((*a=='/')+t,_,a+1)
 :0<t?main(2,2,"%s"):*a=='/'||main(0,main(-61,*a,
 "!ek;dc i@bK'(q)-[w]*%n+r3#l,{}:\nuwloca-O;m .vpbks,fxntdCeghiry"),a+1);
}

believe it or not, that prints out all the verses of "The Twelve Days of Christmas".

:)

edit: forum mangled the include line.
Click to expand...

How the fuck...
 
goto should never be used unless it's absolutely necessary, however it can _ocasionally_ have a couple speed advantages, but nowadays they're pretty much moot and you still should never use it if anybody will be reading your code
 
We had the christmas code on one of my finals as extra credit if you could figure it out. I don't think anyone did (we all went home and compiled it though and were like WTF?).

GOTO's are generally a no-no but there are times when they are appropriate. That example wasn't one of them.
 
if you have a snippet like this:

Code: 
int a[100][100];
int i, j;

...

void main(void) {
	for (i = 0; i < 100; i++) {
		for (j = 0; j < 100; j++) {
			if (a[i][j] == 0) goto end;
		}
	}
	end: printf("done\n");
}

is the goto justified? I think it is.

*edit*
i'm agreeing that gotos are generally bad... this was a question on a final I took a few hours ago, and we had to rewrite this without the goto. (It was a programming language design class)
 
You must log in or register to reply here.
Back
Top