So, a colleague inherited a project to restructure a product..

Sentenryu

He called for my help when he couldn't understand a piece of code. After 2 hours digging, we finally understood what the code does. A convoluted piece of junk that just shows a string on a field on the page. But the most facepalming thing we found was this piece of "code":

public string GetLimitMessage(int total, int limit){
if(total > limit)
try
{
return "The Limit has been reached!"
}
catch(Exception)
{
return "";
}
else
return string.Empty;
}

Aside from translation (the original is on Portuguese), that's the exactly code we found. We are still facepalming.

Kornfeld Eliyahu Peter

Do you know the one wrote this? I want to hire him! My office is too dirty lately... :wtf:

I'm not questioning your powers of observation; I'm merely remarking upon the paradox of asking a masked man who he is (V).

Sentenryu

He doesn't work here anymore (actually can be anyone of 4 people, but none of them work here now). The thing is that all of them bragged about how their code was better and how they were the best I don't want to see the worst. X|

BillW33

Of course they bragged about their code. They used the advanced feature "try-catch" so it must be great code! Well, great in their minds, anyway. X|

Just because the code works, it doesn't mean that it is good code.

Freak30

So I assume all over the codebae you have something like this?

if (GetLimitMessage(a, b).Length > 0)
MessageBox.Show(GetLimitMessaage(a, b));

The good thing about pessimism is, that you are always either right or pleasently surprised.

Nagy Vilmos

Are you mad! That would be far too risky to write it like that, far better would be:

if (!GetLimitMessage(a, b).equals(GetLimitMessage(1, -1)) {
if (!String.IsNullOrEmpty(GetLimitMessage(a, b)) {
MessageBox.Show(GetLimitMessaage(a, b));
}
else
{
MessageBox.Show("The monkey's dead!");
}
}

speramus in juniperus

Sentenryu

It's an APS.NET MVC 3 Application, so it's: Controller:

if(Helper.GetLimitMessage(total, 100)) //Yep, 100 is harcoded on every line it's used...
{
    ViewBag.LimitMessage = Helper.GetLimitMessage(total, 100);
    ViewData\["LimitMessage"\] = Helper.GetLimitMessage(total, 100);
}

View:

@if(ViewBag.LimitMessage != "")
{
     @Helper.GetLimitMessage(total, 100)
}
else if (ViewData\["LimitMessage"\] != null)
{
     @ViewData\["LimitMessage"\]
}

Even my coworker who normally doesn't rant on code is cursing this one.

Sentenryu

Now I need some more mind bleach...

OriginalGriff

Surely a MessageBox is rather "old hat"? How do you display a Fail Whale in a MessageBox?

BobJanova

There isn't a :doh: large enough for this.

Rob Grainger

Woah up there, you're missing a major source of defensive coding here...

try {
if (GetLimitMessage(a,b) != null && GetLimitMessage(a,b).Equals(GetLimitMessage(1, -1))) {
if (!string.IsNullOrEmpty(GetLimitMessage(a,b)) {
try {
MessageBox.Show(GetLimitMessage(a,b));
}
catch (Exception ex) {
throw;
}
}
}
}
catch (Exception ex) {
MessageBox.Show("The monkey's dead!");
}

"If you don't fail at least 90 percent of the time, you're not aiming high enough." Alan Kay.

0bx

Is this the real life? Is this just fantasy? caught in a landslide no escape from reality open your eye's look up to the ceiling and stare I'm just a poor coder, I get no sympathy because I make simple things, make them work respect my deadlines, don't be a jerk Any way management blows, doesn't really matter to meeeee To meeeeeee MAMAAAAA I just killed my coworker put a gun against his head pulled my trigger, now he's dead... :thumbsup:

.

Brisingr Aerowing

:doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh: :doh:

Getting information off the Internet is like taking a drink from a fire hydrant. - Mitchell Kapor