Comment On 1, 1, or 1, or if you really want, 1

Self documenting code. Isn't this what we all should strive for?





p.s. woohoo 1st post

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).

Can anyone say "Copy and Paste"[:^)]

//Load Page Data
    LoadPageData();

now that's good commenting.

Hey this isn't a classic WTF, I cant find it anywhere.

Jake Vinson:

   lblNavigation.text = ddlOperationList.Items.Item(ddlOperationList.SelectedIndex).Text

Apart from everything else, what's wrong with          

lblNavigation.text = ddlOperationList.SelectedValue

to get the text of a drop-down list? Maybe even .ToString it to be extra-sure ... 
 

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.

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.

Volmarias:

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.

I think we found the original developer of this app. [;)]

"You can paint it any color, so long as it's black" - Henry Ford

Anonymous:

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.

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)

Well they could have done it this way:

Dim i As Integer = 0
While i < 10
//Load Page Data
    LoadPageData();
End While

Yes this is a joke[H]

Ken Nipper:

Well they could have done it this way:

Dim i As Integer = 0
While i < 10
//Load Page Data
    LoadPageData();
End While

Yes this is a joke[H]

What's the joke? The fact that you never increment the loop index?

John Smallberries:

Ken Nipper:

Well they could have done it this way:

Dim i As Integer = 0
While i < 10
//Load Page Data
    LoadPageData();
End While

Yes this is a joke[H]

What's the joke? The fact that you never increment the loop index?

Yes.  Now it will load page data indefinitely just to make sure it gets done[:P]

Ken Nipper:

Yes this is a joke[H]

jokes are funny

weak

 "Everyone thinks they have a sense of humor, but then they don't always." -  Steve Martin

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.

Volmarias:

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.

 

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.

Ken Nipper:

 "Everyone thinks they have a sense of humor, but then they don't always." -  Steve Martin

"Some people think something is funny, then expect everyone else to think so too." - John Smallberries

Bytecodes is not codebytes:

Volmarias:

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.

 

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.

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. 

Volmarias:

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.

 

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?

 

Ken Nipper:

Bytecodes is not codebytes:

Volmarias:

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.

 

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.

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. 

I think the meaningful comments got lost someway down in the middle of "original code" and "code-snippet posted at thedailyWTF"

switch (state)
{
    case Enumerations.State.Gollum:
      filthyHobbits.GetPreciousFrom()
      break;
   case Enumerations.State.Smeagol:
      filthyHobbits.GetPreciousFrom()
      break;
   default:
      filthyHobbits.GetPreciousFrom()
      break;
}

Bytecodes is not codebytes:

Volmarias:

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.

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.

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.

Jake Vinson:

switch (state)
{
  case Enumerations.PageState.Edit:
    //Load Page Data

    LoadPageData();

    break;
  case Enumerations.PageState.Update:
    //Load Page Data

    LoadPageData();

    break;
  case Enumerations.PageState.Add:
    //Load Page Data

    LoadPageData();

    break;
  default:
    //Load Page Data

    LoadPageData();

}

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;



}

OneFactor:

switch (state)
{
    case Enumerations.State.Gollum:
      filthyHobbits.GetPreciousFrom()
      break;
   case Enumerations.State.Smeagol:
      filthyHobbits.GetPreciousFrom()
      break;
   default:
      filthyHobbits.GetPreciousFrom()
      break;
}

**applause**

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.

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.

Bytecodes is not codebytes:

Volmarias:

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.

 

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.

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.

Mung Kee:

Jake Vinson:

switch (state)
{
  case Enumerations.PageState.Edit:
    //Load Page Data

    LoadPageData();

    break;
  case Enumerations.PageState.Update:
    //Load Page Data

    LoadPageData();

    break;
  case Enumerations.PageState.Add:
    //Load Page Data

    LoadPageData();

    break;
  default:
    //Load Page Data

    LoadPageData();

}

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;



}

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?

Anonymous:

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?

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



}

For example:

switch (state)
{
  case Person.State.Happy:
    Drink();
    break;
  case Person.State.Unhappy:
    Drink();
    break;
  case Person.State.JusGotDumpedByGirlfriend:
    Drink();
    break;
  case Person.State.JusGotPayRise:
    Drink();
    break;
  default:
    Drink();
}

Mung Kee:

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;



}

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.

Anonymous:

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.

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.

Hmm.  Not removing useless code implies not being professional....



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.



A truly good optimizing compiler should recognize the redundant code and common it up anyway.

This has already been posted.

http://thedailywtf.com/forums/32119/ShowPost.aspx

Mung Kee:

Jakeypoo:

switch (state)
{
  case Enumerations.PageState.Edit:
    //Load Page Data

    LoadPageData();

    break;
  case Enumerations.PageState.Update:
    //Load Page Data

    LoadPageData();

    break;
  case Enumerations.PageState.Add:
    //Load Page Data

    LoadPageData();

    break;
  default:
    //Load Page Data

    LoadPageData();

}

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;
}

 

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.

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();
}

kdd:

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.

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.

DOH! - Make that:
switch (YouDontSucceed)
{
   case At.First:
    Try();
    break;
   case At.Second:
    Try();
    break;
   case At.Third:
    Try();
    break;
  default:
    Try();
}

kdd:

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.

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

Anonymous:

Mung Kee:

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;



}

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.

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.  

dubwai:

This has already been posted.

http://thedailywtf.com/forums/32119/ShowPost.aspx

Only one more day of this $hit Jake

Ken Nipper:

Can anyone say "Copy and Paste"[:^)]

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.

Anonymous:

Anonymous:

Mung Kee:

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;



}

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.

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.  

And you wasted the whole goddamn day writing try...catch blocks

OneFactor:

switch (state)
{
    case Enumerations.State.Gollum:
      filthyHobbits.GetPreciousFrom()
      break;
   case Enumerations.State.Smeagol:
      filthyHobbits.GetPreciousFrom()
      break;
   default:
      filthyHobbits.GetPreciousFrom()
      break;
}

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").

Volmarias:

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.

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

Volmarias:

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.

I can't accept this reasoning. How hard do one just comment out redundant part of the code?

'If Request.Form("ml") = "Y" Then
  lblNavigation.Text = _
    ddlOperationList.Items.Item(ddlOperationList.SelectedIndex).Text
'Else
  lblNavigation.Text = _
    ddlOperationList.Items.Item(ddlOperationList.SelectedIndex).Text
'End If

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.

Anonymous:

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

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:

switch(i)
{
   case 0:
   case 1:
   case 2:
      i = 5;
      break;
   case 3:
      i = 6;
      break;
}

This results in i ==5 if i starts as 0, 1 or 2, and i==6 if i starts as 3.

Drak

Omnifarious:

Bytecodes is not codebytes:

Volmarias:

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.

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.

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.

 

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.