- Feature Articles
- CodeSOD
- Error'd
- Forums
-
Other Articles
- Random Article
- Other Series
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
Self documenting code. Isn't this what we all should strive for?
p.s. woohoo 1st post
Admin
I believe for the page data snippet that it's 'manual edit time polymorphism' - they can come back in and change one of the switches to do something else (not at runtime, but edit time). Follows good programming practices (along with cut-n-paste programming).
Admin
Can anyone say "Copy and Paste"[:^)]
Admin
Admin
Hey this isn't a classic WTF, I cant find it anywhere.
Admin
Apart from everything else, what's wrong with
Admin
To be honest, this isn't all THAT bad.
In both cases, one possible explaination is that at one time, different states did different things. However, they were changed such that they would all do the same thing, with the expectation that they would do different things at some time soon in the future, something that never came to pass.
The first case actually seems more like someone just doesn't understand how switch works, and performed a monkey-see-monkey-do to make the code work.
Admin
Well, if LoadPageData had some sort of way to tell without being passed any information what the page state was, that might make sense...or maybe the programmer was being paid by the line.
Admin
I think we found the original developer of this app. [;)]
Admin
"You can paint it any color, so long as it's black" - Henry Ford
Admin
Cool!
Endless Switch-Statements make you rich!
int s = 0;
switch(s) {
0: //Load Page Data
LoadPageData();
break;
1: // Make Money
dontBotherBreaking();
2: // Make Money
...
default: // You should allways have a default
// Make Money!
}
I'd love to live in that world <:o)
Admin
<FONT color=#000000 size=2>Well they could have done it this way:</FONT>
<FONT color=#000000 size=2>Dim i As Integer = 0
While i < 10
//Load Page Data
LoadPageData();
End While</FONT>
<FONT size=2>
<FONT color=#000000>Yes this is a joke[H]</FONT></FONT>
<FONT color=#0000ff size=2>
</FONT>Admin
What's the joke? The fact that you never increment the loop index?
Admin
Yes. Now it will load page data indefinitely just to make sure it gets done[:P]
Admin
jokes are funny
Admin
weak
Admin
Admin
Actually, the later one isn't too bad, and there are reasons why someone would want to do it like that, like if they want it to do said action if the variable isn't set, or set to Y. It might be that for all other cases (variable set to non-zero, non-Y value) that another action should take place, or this one shouldn't. Still, it could have been condensed onto one line.
Admin
Spot on! I think both cases is something like you suggest. There is no WTF in this code, the big WTF here is how little imagination people here have (except Volmaris), and obviously can't see the forest behind all the tree's.
Admin
"Some people think something is funny, then expect everyone else to think so too." - John Smallberries
Admin
I would agree that if functionality was changed due to differing requirements, then this would make at least some sense. If that is the case though, then the WTF, in my opinion, is the lack of meaningful comments regarding the change.
Admin
Even if that is what happened, everyone reading the code is going to have to take the time to determine that the cases are the same. When the requirement comes back, how long does it really take to add an if statement back in?
Admin
I think the meaningful comments got lost someway down in the middle of "original code" and "code-snippet posted at thedailyWTF"
Admin
switch (state)
{
case Enumerations.State.Gollum:
filthyHobbits.GetPreciousFrom()
break;
case Enumerations.State.Smeagol:
filthyHobbits.GetPreciousFrom()
break;
default:
filthyHobbits.GetPreciousFrom()
break;
}
Admin
Uh, huh. So, I should add in lots of unecessary classes and extensive inheritence hierarchies just in case I might need them someday?
Extra code is extra code that has to be read and udnerstood. If the person wanted to be clear that all the cases were covered (s)he could've just put in all the cases before the two lines of code that were needed. As it is, you have to do a whole bunch of reading to realize all the cases are the same, then you want to go strangle the original programmer. It's a bad coding practice.
It's even worse than having large swaths of commented out code (just in case you need to put it back in) when the code is under version control. At least then most reasonable editors flag all the dead code as being a comment.
Admin
I have seen, on many occassions, people do this very same thing with exception handling. For example:
try{
...
}catch(IOException ioe){
log.debug("There was a IOException", ioe);
throw ioe;
}catch(SAXException saxe){
log.debug("There was a SAXException", saxe);
throw saxe;
}catch(NumberFormatException nfe){
log.debug("There was a NumberFormatException", nfe);
throw saxe;
}
Admin
on another note, I have, on occasion, used a switch-case with lots of cases with the same code, and only the default doing anything different, purely 'cos it appealed to my sense of aestetics better than a mess of if(foo==a||foo==b||...). But then, that's what case fall-through is for, so that wouldn't even begin to explain the ^C^Ving going on here.
Admin
well, cack. bitten by not previewing 'cos the CAPCHA didn't work on preview. that's the last time i don't log in out of laziness. I still think it's distinctly daft that this forum software continues to be used here.
Admin
There's no doubt that there is a WTF here. If you change the code, and that code change renders the surrounding code obsolete, remove the surrounding code. In your scenario, the WTF is that they didn't remove the useless switch. After all, we ARE supposed to be professionals.
Admin
on a try { that throws NumberFormatException, SAXException, IOException, and FourthException, FifthException, SixthException, how do you catch the first three and let the last 3 propagate?
Admin
Just don't re-throw the first 3. (changed to log.error() too)
try{
...
}catch(IOException ioe){
log.error("There was a IOException", ioe);
//dont re-throw
}catch(SAXException saxe){
log.error("There was a SAXException", saxe);
//dont re-throw
}catch(NumberFormatException nfe){
log.error("There was a NumberFormatException", nfe);
//dont re-throw
}catch(FourthException fourth){
log.error("There was a NumberFormatException", nfe);
throw fourth; //DO re-throw
}
Admin
For example:
Admin
At university, one of our lecturers taught us that this is the correct way to do it, so that you can clearly see which exceptions are being caught there.
A certain tutor then marked down nearly the entire class for doing this on an assignment.
Admin
The same case can be made for the original posting ( switch (state) ), the make clear what possible values "state" can be. I say, who the hell cares if you going to do the same thing with it.
Admin
Hmm. Not removing useless code implies not being professional....
<checks some="" code="" written="" by="" 'professionals="" for="" obvious="" lack="" of="" utility="">debug = false;
...
if (debug) {
...
}
Bloody amateurs.
Seriously, while that code is not exactly a shining example of intelligence others have proposed various reasonable origins. Your time/effort is a resource when programming, and good programmers know to both optimize last and also to focus optimization on the important, often-executed code, which this may well not be.
</checks>
A truly good optimizing compiler should recognize the redundant code and common it up anyway.
Admin
This has already been posted.
http://thedailywtf.com/forums/32119/ShowPost.aspx
Admin
About exception code snippet :
1. If the WTF is doing "log.debug" instead of "log.error", I agree.
2. Are you saying that it should be "catch (Exception ex)"?
3. Individual exception handling is preffered (if you know what to do with it) although you may handle them in the same way.
4. So this may make sense depending on the context. (i.e. This method may be in a middle layer, but the caller of this method may have to re-catch the same exceptions and dispaly a proper user friendly message in the UI layer).
Original post :
It is a horrible WTF.
Just comment out the damn code when you are not using. And put some meaningful comments.
If you can't do that, you are just a lazy programmer.
Admin
Well, I think it is sound axiomatic advise, let alone programming:
switch (YouDontSucceed)
{
case At.First:
Try();
break;
case At.Second:
Try();
break;
case At.Second:
Try();
break;
default:
Try();
}
Admin
Wrong answer on commenting out code. If you don't need it, then take it out. That's what source history tracking is for.
OTOH, agreed on the meaningful comments and this being a horrible WTF.
Admin
DOH! - Make that:
switch (YouDontSucceed)
{
case At.First:
Try();
break;
case At.Second:
Try();
break;
case At.Third:
Try();
break;
default:
Try();
}
Admin
1. Typo on my part, and corrected in a later post.
2. Yes
3. Not across application layers.
4. It's generally not good design to throw multiple types of exceptions across layers. If the layer is truly a 'black box', you certainly won't see it's public interface methods throw SAXExceptions.
Original post :
Lazy? Belee dat
Admin
Well, if you applied that strategy everywhere then you indeed should have lost marks. Suppose your call chain is e() calls f() calls g() calls h(). Now suppose h() throws those 3 exceptions, so in g() you put your above construct around the call. Thus g() now throws those 3 exceptions, and so in f() you also put your above construct around the call to g(). Same thing for the call to f() in e(). Now your exception is logged 3 times when it should be logged only once.
Admin
Only one more day of this $hit Jake
Admin
and getting distracted before changing the pasted code to it's sightly different, final form.
I had stuff like that happen to me during coding, but it usually showed up in testing. In this case, the developer did obviously not test his code.
Admin
And you wasted the whole goddamn day writing try...catch blocks
Admin
Perfect example. Because at some point, the maintenance coder has to go in and change the Smeagol case to check if the Gollum state has been hit recently. If not, do filthyHobbits.TryToPleaseTheSkinnyOne(), otherwise .GetPreciousFrom(), then later it has to go back to its original state.
And for all the talk about professionals refactoring, some of us professionals consider the maintenance guy as well. It's always a trade off. Perfect elegant code is great right up until the point the requirements change (known to much of the world by it's common name, "delivery").
Admin
If the language is C#, the WTF is that they used a swithc instead of an if. C# doesnt allow fall-through like C++.
so C++ lets you do case A: case B: case C: // more readable than c# version
but C# forces you to do case A: break; case B: break; case C: break;
should use an if and say if(A || B || C) // more readable
Admin
I can't accept this reasoning. How hard do one just comment out redundant part of the code?
And that would a lot more eaily to follow in an editor.
I often see this kind of branch redundant code when I maintaining someone else written code. It means that the original programmer is lacking proper programming skills, which their are hard to follow the logic, full of bugs and very difficult to maintain and extend. Then I have no choice but refactoring everything they wrote. This at least double the amount that what I should do.
Admin
Are you sure C# doesn't have a better method? VB.NET uses the following:
select case (something)
case A, B, C: dostuff
case D, E: dootherstuff
end select
I'm sure C# has a similar construct, ad after testing, it looks like it works exactly the same as the C++ variant:
<FONT color=#0000ff size=2>switch</FONT><FONT size=2>(i)
{
</FONT><FONT color=#0000ff size=2>case</FONT><FONT size=2> 0:
</FONT><FONT color=#0000ff size=2>case</FONT><FONT size=2> 1:
</FONT><FONT color=#0000ff size=2> case</FONT><FONT size=2> 2:
i = 5;
</FONT><FONT color=#0000ff size=2>break</FONT><FONT size=2>;
</FONT><FONT color=#0000ff size=2>case</FONT><FONT size=2> 3:
i = 6;
</FONT><FONT color=#0000ff size=2>break</FONT><FONT size=2>;
}</FONT>
<FONT size=2>This results in i ==5 if i starts as 0, 1 or 2, and i==6 if i starts as 3.</FONT>
<FONT size=2>Drak
</FONT>Admin
Obviously my intention with my post was not that this isn't bad coding practise. But this code-snippet alone says so much why it is there. This stuff is quite often in my own code, but only temporary, I have a failsafe method of keeping track of where I do these temporary "test"s, so I always remove them, rewrite them, bring em back to original or whatever I do just as soon as I have completed my test. The tests could be anything, like testing a new function in all different application states and so on. It saves so lot of time that in just some seconds be able to do a test, in creating something like this. The clearly thing about this guy is that he lack practise in how to keep track of them, so he obviously missed to correct this test. If this code is in function that is never used and never will be used, we have no clue. Then he again saved time by not correcting it, but he should of had time to remove it from the project. But if he is only on this project, and no one ever will "take over" after him, I think this code-snippet illustrates "lack of time" a LONG LONG way.