|
FINALLY!!!!
My first entry on a coding horror!
Is this a sign I am starting to learn, possibly, but even in goood old fashioned ms basic circa 1981 I would have spotted this load of old codswallop!
So pleased to say that I would never have done this!
------------------------------------
Happy Primes Lead to Happy Memories.
Don't Google FGI
|
|
|
|
|
heheheh . Cool thing....i myself as developer can understand that sort of things can happen when code passes through certain hands without proper comments.
|
|
|
|
|
Whether it was the basement of great code or the ruin of an unlucky project. Who knows... 
A good optimized cempiler would clean this up. 
Greetings from Germany
|
|
|
|
|
omg.. what else is there to say ?? 
Michael Davey
biproject.com rss and blog news in a more palatable format
mobile edition now available!
|
|
|
|
|
- Pascal - wrote: We found it very funny
I find it quite useful, actually. This is the sort of thing that sits in code that is prone to change. Why destroy the entire structure when a month or two from now, someone has a need to add in an index of 2 and 3?
Though, admittedly, I'd comment out everything but the 'do something' code when it got this low. But I'd still leave it THERE.
|
|
|
|
|
I was cleaning up the code in a parser. There is a switch/case statement that spans over 2,200 lines. I removed a bunch of exits from various case statements and moved it up as a global test on the errorCode.
It just didn't want to exit when the errorCode was set.
state=1;
do
{
result = token( tokstr, FALSE, NULL, FALSE);
switch(state)
{
if (errorCode != 0)
return FALSE;
case 1:
switch(result)
{
}
} while( result != Keyword::EOFTOK );
“Although distant in time, Homobonus does in fact figure as a saint for the Church and society of our time.”
-- Pope John Paul II
|
|
|
|
|
2200 lines of cases? Impressive, at least to me, humble employee of small companies.
|
|
|
|
|
Oshtri Deka wrote: 2200 lines of cases? Impressive, at least to me, humble employee of small companies
I'm not happy with 2200 lines of cases. I'm seeking to clean this up and reduce complexity. Things tend to just keeping growing.
I'm curious, do you see the problem with my posted code?
|
|
|
|
|
I was sarcastic, though I am impressed that someone wrote so many lines of code for one switch-case block. What does that application do?
checking before first case?
|
|
|
|
|
Oshtri Deka wrote: I was sarcastic, though I am impressed that someone wrote so many lines of code for one switch-case block. What does that application do?
That module has to do with parsing a parameter file. It probably was state-of-the-art in 1983. It has just grown and grown. We need an option at to handle FOOBAR? No problem, add a #DEFINE FOOBAR 1742 and one more case statement.
Continue the process for almost a quarter century.
The application classifies inpatient hospital records into disease categories and stages of the disease. The portion of the program that does that has been modernized.
checking before first case?
Yeah, that code was unreachable. I checked it into source control. It worked great if there were no errors. 
|
|
|
|
|
You should construct a data structure and keep the possible values in case statements in an ordered way in your data structure; then your incoming value should be compared with the values inside your data structure in binary search fashion...
this will be faster and save you from time consuming sequential searching
|
|
|
|
|
xenonysf wrote: You should construct a data structure and keep the possible values in case statements in an ordered way in your data structure; then your incoming value should be compared with the values inside your data structure in binary search fashion...
this will be faster and save you from time consuming sequential searchin
I agree with what you say but not for the reasons you give.
A switch/case statement has better performance than a sequence of if/else statements. The compiler may generate a jump table. For large tables, some use binary searches. See http://www.eventhelix.com/RealtimeMantra/Basics/CToAssemblyTranslation3.htm[^].
So performance isn't the driving factor here.
But I do agree that the cases -- where possible -- should be encapsulated in a data structure. We would like to have actions for the various cases in a data structure to simply the logic.
Complexity leads to increased probability of error. We have unneccessary complexity. However, it takes a while to untangle code that has grown for decades.
|
|
|
|
|
switch(state)
{
if (errorCode != 0)
return FALSE;
case 1:
switch(result)
{
My guess is that execution jumps to one of case labels and skip the if statement entirely?
[ My Blog] "Visual studio desperately needs some performance improvements. It is sometimes almost as slow as eclipse." - Rüdiger Klaehn "Real men use mspaint for writing code and notepad for designing graphics." - Anna-Jayne Metcalfe
|
|
|
|
|
My compiler woud throw exception
|
|
|
|
|
bsaksida wrote: My compiler woud throw exception
Are you sure? I think it's legal C++ albeit stupid programming. I wish it had thrown an exception. But Visual Studio was happy with it, as were the compliers on AIX, Solaris, and HP/UX.
|
|
|
|
|
why is there a switch inside switch
This style of progrmaing introduce more compiling errors than the bugs
|
|
|
|
|
bsaksida wrote: why is there a switch inside switch
Well, it fits the logic. When the parser is starting a statement, the state is 1. Then it does a switch statement based on the first token.
The code all works, I'm just not happy with how complicated it is. I used SourceMonitor[^] to get a metric on the complexity.
Mike Swanson[^] suggests that <quote>Anything with a greater complexity than 10 or so is an excellent candidate for simplification and refactoring. Well, the code in question has a complexity number of 638 as measured by SourceMonitor.
You don't go from 638 to 10 in one step. So one of the things I did was to have a single exit with the error code at the top of the case statement. Or at least, I tried to. My logic error was a little embarrassing.
But an error like that is too good to keep to myself. I had to share it.
|
|
|
|
|
I have found, on several places in our app, code similar to this:
try<br />
{<br />
}catch{}
Do I need to say I have found that during urgent debugging session (important customer had had complaints)
Empty catch blocks, and most of them were in methods which deal with data.
Let's just say that we use multi-threading and reflection, a lot, and I spent too much time on only localizing this.
Why are people so reckless? I am far from being expert, but everyone who likes to think about oneself as a coder or developer should use common sense.
Notice: Somehow (perhaps in a state of fury) I have forgotten to describe places where I had found above mentioned problem, and that has made this post is a bit misleading.
Programmer who have written that code, used this "technique" only to sweep things under the rug!
No returning values, filling dataSets, no validation, allowing bad user input (constraint violations)... in short error-exception domino which occurs only in specific situations.
I am aware that empty try-catch block isn't excuse for witch hunt, but (at least) generally it isn't best practice. When I am not sure if something is preferable, I ask more experienced colleagues.
-- modified at 6:31 Tuesday 20th November, 2007
|
|
|
|
|
C'mon, that's just the C# way of saying
On Error Resume Next
|
|
|
|
|
Nemanja Trifunovic wrote: C'mon, that's just the C# way of saying
On Error Resume Next
Come to think of it, almost any VB6 error handling code would belong here.
Nathan
|
|
|
|
|
I wish it was that simple.
|
|
|
|
|
Actually it's worse, more like On Error Goto ...
WarePhreak
Programmers are tools to convert caffiene to code.
|
|
|
|
|
Oddly enough, I ran into a place where I have to use this in a WPF app I'm working up for my boss. In code, if you try to load an Image control, you can't just give it a filename, you have to give it an ImageSource object. The most obvious of these is BitmapSource, which inherits from ImageSource. BitmapSource won't take a filename either - it requires a Uri object, which is (as its name implies) a URI to a file somewhere.
BitmapSource has one strange little caveat. Its properties are immutable. Nothing you change will stick unless you explicitly call the BeginInit method on it. Once you're done, call EndInit and all your changes are persisted.
Herein lies the problem. My application calls BeginInit on a BitmapSource and runs through a switch that looks at the application state to decide which image to load. If the switch detects an invalid state, it sets the Uri to null (so that no images load). Then, at the end of the switch, I call EndInit and my BitmapSource should have loaded either an image file or nothing at all.
Here's the kicker. BitmapSource throws an exception if you call EndInit without setting a Uri. (Well, a Uri or a StreamSource, but I'm not dealing with streams in my app.) So in the case where the switch didn't load an image in its default case, my BitmapSource blows up.
Unfortunately, whoever designed the interface that ImageSource implements (the one that contains BeginInit and EndInit) didn't seem to realize this fact, and neglected to include an AbortInit method. In other words, once you call BeginInit, you have to load an image somehow or catch the error that results from an empty URI. 
So yeah, my method now looks like this:
imgLoader.BeginInit();
switch(stateVar)
{
default:
imgLoader.UriSource=null;
break;
}
try
{
imgLoader.EndInit();
}
catch { }
Sad, isn't it?
Please don't bother me... I'm hacking right now. Don't look at me like that - doesn't anybody remember what "hacking" really means?
|
|
|
|
|
I might be misunderstanding, but shouldn't you still have some code in the catch block to ensure that your stateVar variable is invalid. Otherwise you won't know about exceptions when the exception is meaningfull?
|
|
|
|
|
Not really; the only invalid stateVar that should get caught by the default case is 0 (i.e. don't display any images). There's a handler that should set the Image object to null, rather than its URI in this case, but there's still a couple of cases where the application changes to state 0 and runs the switch statement instead. (These cases shouldn't exist but I didn't write up a full state diagram because this application is a tech demo my boss wants to present to his peers and he doesn't want me to spend a lot of time on it. Go figure.)
Please don't bother me... I'm hacking right now. Don't look at me like that - doesn't anybody remember what "hacking" really means?
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
