Assume one common_event and two possible rare_events.
If coded thusly . . .
IF common_event ORIF rare_event1 ORIF rare_event2
WRITE_AUDIT code
ENDIF
. . . the compiler would generate the following eight machine language instructions:
Test for common_event
Branch to WRITE_AUDIT if condition met
Test for rare event1
Branch to WRITE_AUDIT if condition met
Test for rare event2
Branch to EXIT if condition not met
WRITE_AUDIT: code
EXIT: code
RESULTS
If common_event true: 4 instructions executed
If only rare_event2 true: 8 instructions executed
If no cases true: 6 instructions executed
Coded your way, the following 14 machine language statements would be generated:
Set flag to false
Test for common_event
Branch to TEST2 if condition not met
Set flag to true
Branch to WRITE_AUDIT
TEST2: Test for rare_ event1
Branch to SETFLAG if condition met
TEST3: Test for rare_ event2
Branch to WRITE_AUDIT if condition NOT met
SET_FLAG: Set flag to true
WRITE_AUDIT: Test flag
Branch to EXIT if flag not on
write_audit code
EXIT: code
RESULTS
If common_event true: 9 instructions executed
If only rare_event2 true: 12 instructions executed
If no cases true: 10 instructions executed
Your best case scenario executes more machine instructions than the worst case scenario using a compound IF statement. (I’m not saying that the single compound IF statement is the best way to code here – I’m using it to demonstrate efficiency.)
Your grade:
Consistency of style: F
Maintainability: D
Efficiency: F
Elegance: F
Impressing anyone who’s been coding for more than a couple of weeks: F I have, however, enjoyed the geek fest.
It has many extra comparisons, and will execute more slowly.
True, although again that’s going to be marginally less efficient, execution-wise.
I think the game industry is one of few places where even rare GOTOs are OK (and remember, I’m not FOR them, as I’ve only used them once or twice in 10 years), in that (a) performance is always paramount, (b) code rarely has to hang around for 15 years, and (c) bugs are never life-threatening.
Eh. I’m used to that style now. And if you change customer_id from an integer to a string, then it’s USEFUL to change it’s name so the compiler will automatically find all occurrences of it for you, so you can make sure you’re not now doing something that no longer works since its type has changed
If you change something from an integer to a string, the compiler will probably point out all its uses even if the name doesn’t change. The only case where you’d have to worry about something like that is if you’re changing from one fundamental type to another.
IMO, the type of a variable should be obvious from its name (and perhaps a little bit of domain expertise). If not, it’s most likely poorly named.
I suppose I could use actual structured exception handling, but that would be overkill: no data needs to be transmitted with the exception and the error never, ever gets propagated out of the function scope, so why incur the overhead of the -fexception flag? Copying the error-handling code to every location where I might put a goto would be a maintenance nightmare, and wrapping it in a function would require pushing who knows how much data onto the stack. If the language I’m using has labelled blocks and labelled break statements (like Perl for example) then I can usually avoid even this much use of the goto statement, but in C++ this is sometimes the simplest, cleanest solution.
One of the problems with hungarian notation is that that variable types tend to NOT be changed consistently when the underlying type changes, and that creates misleading code. This happens a lot with types that are common shorthand. Is wParam always guaranteed to be a word? Nope. But it’s still called wParam.
I prefer code that when read describes the logical structure of the program. The underlying type of the variable is an implementation detail that is better left hidden.
I prefer:
if (customerName.length > max_size)
errorText = TOO_LONG;
to
if (lpcstrCustomer.length > nSize)
lpszrerrText = lpszTOO_LONG;
Here’s an example from Charles Simonyi’s article advocating Hungarian in the MSDN knowledgebase:
#include "sy.h"
extern int *rgwDic;
extern int bsyMac;
struct SY *PsySz(char sz[])
{
char *pch;
int cch;
struct SY *psy, *PsyCreate();
int *pbsy;
int cwSz;
unsigned wHash=0;
pch=sz;
while (*pch!=0
wHash=(wHash<>11+*pch++;
cch=pch-sz;
pbsy=&rgbsyHash[(wHash&077777)%cwHash];
for (; *pbsy!=0; pbsy = &psy->bsyNext)
{
char *szSy;
szSy= (psy=(struct SY*)&rgwDic[*pbsy])->sz;
pch=sz;
while (*pch==*szSy++)
{
if (*pch++==0)
return (psy);
}
}
cwSz=0;
if (cch>=2)
cwSz=(cch-2/sizeof(int)+1;
*pbsy=(int *)(psy=PsyCreate(cwSY+cwSz))-rgwDic;
Zero((int *)psy,cwSY);
bltbyte(sz, psy->sz, cch+1);
return(psy);
}
Bear in mind that this is an example of GOOD hungarian. I think it sucks. I also hate his indenting style.
Having had to decipher a several-page long optimization algorithm that made heavy use of GOTO (in Fortran), I would have to say GOTO is pretty horrible. Enclosed loops are so much easier on the brain, and force you to organize your process. And if your code is going to be used and expanded by others in the future, using GOTO statements can be downright rude and inconsiderate.
And what if I have many more loops, both nested and sequential? I would have to check the error flag at the beginning of every loop (well, every loop after the first point where the error could occur), in order to ensure that the error propagates out of the nested loops correctly. What if errors are truly an exceptional case, and 99.9% of the time the error handling code goes unused? And what if this is a heavily-used portion of the code as well? Why should I be inserting “!error &&” at the beginning of every loop to deal with an exceptional condition?
I also don’t see what is hard to maintain about this example. Sam Stone has made a good argument about maintaining spaghetti logic, but in his own words:
My code fragment satisfies this condition nicely. The error-handling code is isolated from the main path of the algorithm, and its position immediately after the main return statement clearly identifies it as code which is only executed in exceptional conditions. Jumps to the error-handling code are also clearly identifiable.
Maintainable? The error-handling code, being isolated, is easy to alter, and new calls to the error-handling code are easy to insert. Moreover, if it becomes necessary to modify some other part of the algorithm that doesn’t use the error-handling code, then that can be accomplished without the new programmer worrying about breaking the error-handling mechanism. Whereas with an error flag and a break statement, any change to the algorithm, anywhere, can break the error-handling mechanism even if the change is to a part of the algorithm that doesn’t use the mechanism, if the programmer forgets to include that all-important “!error &&”.
As I said, if C++ had labelled loops and labelled break statements like Perl then that would be a better solution than mine; likewise if I don’t mind the overhead of structured exception handling (if I’m already using it somewhere else, for example) then that would be a better solution too. But this solution is low-overhead, clear, and encapsulates the error-handling code nicely.
I use GOTO logic, but without GOTO statements. I just find that it makes the code more readable in certain situations, and also for the reasons Orbifold cites. Here’s an example, and I won’t mind if somebody tears it apart for the sheer number of coding standard violations, as I think I have more violations than lines of code. Still, to my mind, this code is compact and easy to maintain:
Private Function CheckDirty() As Boolean
Dim i As Long
CheckDirty = True
Do While CheckDirty
i = lstEmployee.ItemData(.lstEmployee.ListIndex)
If StrComp(txtFirst.Text, mEmp(i).First, vbBinaryCompare) <> 0 Then Exit Do
If StrComp(txtLast.Text, mEmp(i).Last, vbBinaryCompare) <> 0 Then Exit Do
If mEmp(i).Support <> (chkSupport.Value = 1) Then Exit Do
If mEmp(i).Train <> (chkTrain.Value = 1) Then Exit Do
If mEmp(i).Setup <> (chkSetup.Value = 1) Then Exit Do
If mEmp(i).Teardown <> (chkTeardown.Value = 1) Then Exit Do
If Val(txtLaptops.Text) <> mEmp(i).Laptops Then Exit Do
CheckDirty = False
Loop
If mblnDirty <> CheckDirty Then
mblnDirty = CheckDirty
EnableToolbar
End If
End Function
This example is blatant GOTO logic, though there are no GOTO statements in it. What say you on how easy/difficult this code (and others examples of its ilk) would be to maintain?
I’ve seen (Java) code that worked like that, that was hideous. Yours is small enough and only does one thing per line so it doesn’t matter. BUT, the sucker I had to deal with was a full ass honking game written into a single do {} while about 5000 lines long that got looped over, and any time he felt like he was done, he would just go “break;”. ERGGGHHHHHHH NO DO THAT!!!
while (game) {
do {
drawBackground();
if (status == 1) {
drawMenu();
break;
}
else if (status == 2) {
drawMap();
}
updatePositions();
if (status != 3) {
moveCar();
break;
}
}
while (0);
}
NWOOOOOOOOOOO!!!
No comments either, nor functions–unlike in the example above, which does have functions. He just copied and pasted the same code in three or four times as needed in various random places.
I’m a big fan of GOTO and as others have stated, I cannot see the hatred for it.
Maybe the reason for its strict prohibition, is that it is easily enforceable.
“Hey, you used a GOTO in this program. Don’t do that !!”
I am also a firm believer in comments in programs. But unlike using GOTO, criticizing inadequate commenting styles is somewhat tougher to do.
“Hey, you know the rules - 1 line of comments for every fifty lines of code.”
Incidentally, JavaScript has eschewed the GOTO command entirely. Which other programming languages have adopted this unseemly practice?
FWIW - the real code looks more like one of Sam Stone’s good examples (the one with a stream of boolean variable checks) than the pseudo code I’ve posted.
Phew. I didn’t really intend this as a code review y’know.
