Coding Horrors: You can gather all your technical debt into one easy framework!

The Something Awful Forums > Discussion > Serious Hardware/Software Crap > The Cavern of COBOL > Coding Horrors: You can gather all your technical debt into one easy framework!

«‹›1501 »

Thom Yorke raps: Nov 2, 2004

I remember there used to be a thread like this, but I can't find it anymore. Just post code that either you or your coworkers/classmates have written that is hilariously bad.

I've been looking through a large system recently, since I just started a new co-op and I have to get caught up. This one coder (who I've never met) has stuff like this in all his classes:
private static final int FIVE = 5;
private static final int NEG_ONE = -1;
private static final int FIVE_BILLION = 5000000000;
private static final String SPACE_PIPE_EQUALS = " |=";
private static final String ERROR_WITH_CODE = "Error with code";

I was reading one of his classes today that had 50 lines of constants written like that. In an 800 line class. Also, in multiple places he does this:
File fw = new File(args[0]);
File fh = new File(args[1]);
File nv = new File(args[2]);
File nh = new File(args[THREE]);
File tw = new File(args[FOUR]);

He does this in multiple classes. 0, 1, 2 are fine, but any number higher than that he replaces with a named constant. He also doesn't comment his code, and uses method names like "task" and "task2". Then there was this method:

code:

private boolean compareInts(int one, int two)
{
    boolean areEqual = false;
    if(one == two)
    {
        areEqual = true;
    }
    return areEqual;
}

His code is unreadable, and I am considering begging my boss to let me rewrite it, since he doesn't have me doing anything else. Of course, if I do that then I need to try to understand what exactly his code does, which may be impossible.

I'm sure compared to many of you that this isn't all that bad, so share your own tale of horror!

# ? Mar 21, 2008 00:33

Adbot: ADBOT LOVES YOU

# ? Apr 25, 2024 13:53

CeciPipePasPipe: Aug 18, 2004; This pipe not pipe!!

How come the guy is still employed?

# ? Mar 21, 2008 00:51

very: Jan 25, 2005; I err on the side of handsome.

Don't you know the virtue of avoid magic numbers in your code?

# ? Mar 21, 2008 01:03

Thom Yorke raps: Nov 2, 2004

CeciPipePasPipe posted:

How come the guy is still employed?

My guess is that he is/was a co-op. They were apparently rather excited to hire me, and all I did was answer some pretty basic questions on OOP (not saying this to brag, because I am not that good of a programmer), so they probably don't have the most talented pool to choose from. Hell, they're hiring from my school, I know they don't have that talented a pool to choose from. When I mentioned the lovely naming to the senior developer, he said he had not seen it, so probably no one really noticed.

# ? Mar 21, 2008 01:14

ehnus: Apr 16, 2003; Now you're thinking with portals!

code:

class SomeClass
{
public:
    SomeClass( const SomeClass& )
    {
        assert( 0 && "This constructor should not be called" );
    }
};

# ? Mar 21, 2008 03:22

blackmomba: Feb 11, 2005; :snake:

I worked at a place that used SQR that i had to convert to PERL. The SQR code was used to output reports and random information from the sybase server. It was some of the worst code I have ever seen. The code had methods that were in no particular order and the main method was spread throughout the document. The developer used no variable scope (A variable would be declared in one method and used in a different one without passing it). To make things worse there was embedded sql that would query entire tables where only a few fields were needed.

Don't ever agree to learn SQR! It was the worst decision of my life.

Oh and this isn't a coding issue but the code was separated into different files (an attempt at code reuse), but the files were spread across three different version control servers and there was no way to tell if you were working on the most current version.

# ? Mar 21, 2008 05:15

Evil Robot: May 20, 2001; Universally hated.; Grimey Drawer

code:

assert( 0 && "This constructor should not be called" );

I prefer this:

code:

assert(!"This constructor should not be called");

It gives the newbie programmer a little bit more pause to admire my glory.

# ? Mar 21, 2008 06:40

ashgromnies: Jun 19, 2004

very posted:

Don't you know the virtue of avoid magic numbers in your code?

Are you being sarcastic? Please please please be being sarcastic.

const int THREE = 3;

is just as bad as using 3 everywhere.

It should be

const int NUM_CHOICES = 3;

or whatever 3 represents.

# ? Mar 21, 2008 16:31

ohgodwhat: Aug 6, 2005

But THREE is more descriptive....

# ? Mar 21, 2008 18:07

skidooer: Aug 6, 2001

From a Rails application:

code:

...
eval("@image."+params[:model]+"_id ='"+params[:model_id]+"'") if params[:model_id]
...

# ? Mar 21, 2008 18:09

dwazegek: Feb 11, 2005; WE CAN USE THIS

Ranma4703 posted:

code:

private boolean compareInts(int one, int two)
{
    boolean areEqual = false;
    if(one == two)
    {
        areEqual = true;
    }
    return areEqual;
}

I have a colleague who also does stuff like this. For whatever reason, he refuses to use return anywhere, except the last line of a method. This frequently results in methods with absurd amounts of indentation, or stuff like this:

code:

public bool HasValidItems(SomeClass[] items)
{
  bool retVal = false;
  foreach(SomeClass item in items)
  {
    retVal = retVal || item.IsValid;
  }
  return retVal;
}

public SomeClass FindSomeItem(SomeClass[] items)
{
  SomeClass retVal = null;
  foreach(SomeClass item in items)
  {
     if(item.MatchesWhateverWeAreLookingFor)
     {
       retVal = item;
     }
  }
  return retVal 
}

Who cares if we've already found what we're looking for, let's keep on looking :cool:

# ? Mar 21, 2008 18:26

very: Jan 25, 2005; I err on the side of handsome.

ashgromnies posted:

Are you being sarcastic? Please please please be being sarcastic.

Haha. Yeah.

# ? Mar 21, 2008 18:37

Steampunk Mario: Aug 12, 2004; DIAGNOSIS ACQUIRED

dwazegek posted:

I have a colleague who also does stuff like this. For whatever reason, he refuses to use return anywhere, except the last line of a method. This frequently results in methods with absurd amounts of indentation, or stuff like this:
code:
public bool HasValidItems(SomeClass[] items)
{
  bool retVal = false;
  foreach(SomeClass item in items)
  {
    retVal = retVal || item.IsValid;
  }
  return retVal;
}

public SomeClass FindSomeItem(SomeClass[] items)
{
  SomeClass retVal = null;
  foreach(SomeClass item in items)
  {
     if(item.MatchesWhateverWeAreLookingFor)
     {
       retVal = item;
     }
  }
  return retVal 
}
Who cares if we've already found what we're looking for, let's keep on looking

It's an application of the Single Function Exit Point philosophy, with which I don't agree. I think it's good for some issues, especially resource management, but largely you want to use RAII for that anyway, and code can get very messy if you can't bail out early (goto, anyone?).

# ? Mar 21, 2008 18:50

csammis: Aug 26, 2003; Mental Institution

dwazegek posted:

code:

public bool HasValidItems(SomeClass[] items)
{
  bool retVal = false;
  foreach(SomeClass item in items)
  {
    retVal = retVal || item.IsValid;
  }
  return retVal;
}

public SomeClass FindSomeItem(SomeClass[] items)
{
  SomeClass retVal = null;
  foreach(SomeClass item in items)
  {
     if(item.MatchesWhateverWeAreLookingFor)
     {
       retVal = item;
     }
  }
  return retVal 
}

This would be perfectly fine if he had ever heard of the "break" keyword :sigh:

# ? Mar 21, 2008 18:52

NC Wyeth Death Cult: Dec 30, 2005; He lost his life in Chadds Ford, he was dancing with a train.

I can't post it because I don't have access to it any more, but I was recently contracted to do an audit on the code of a custom mailing list program after the company doing it said (after a year of "development") that MySQL could only handle around 50,000 emails and that to handle the 450,000 expected ones they would have to upgrade the server to Oracle.

The program itself was out of my league so I subbed it out to a friend who is an amazing programmer and he would send me examples of the security like "12345" type passwords and completely illegal and actionable stuff like the inability to opt out of the list.

Other things he found going through it:

-database password was wrong
-No error checking on mysql_connect, mysql_query, or much of anywhere actually.
-Opt out wasn't just broken, it just didnt' work.

"when it displays the results of the filter/search it omits the people marked as opted-out. Buuuuuuut, when it processes to send email it doesn't."

This is how my friend described the entire project:

"The site is like....a kid knows his multiplication tables, but that doesn't mean he knows how to apply that to solving applied calculus. The guy that did this site knows that nuts fit on bolts, but can't build a truss."

I have been cleaning up after this development company and their big product is this all encompassing content management system that would take someone three months to learn how to create learning PHP/MySQL from scratch.

# ? Mar 21, 2008 19:37

hey mom its 420: May 12, 2007

dwazegek posted:

code:

public bool HasValidItems(SomeClass[] items)
{
  bool retVal = false;
  foreach(SomeClass item in items)
  {
    retVal = retVal || item.IsValid;
  }
  return retVal;
}

Also what the hell is up with setting a variable to false and then doing var = var || x
Every kid knows that false || x equals to x (in a boolean context)
He could have mine as well written that line as

code:

retVal = ((retVal && false) || (false || ((retVal || item.IsValid) && true))) && item.IsValid;

# ? Mar 21, 2008 20:08

Avenging Dentist: Oct 1, 2005; oh my god is that a circular saw that does not go in my mouth aaaaagh

Bonus posted:

Also what the hell is up with setting a variable to false and then doing var = var || x
Every kid knows that false || x equals to x (in a boolean context)
He could have mine as well written that line as

Some point along the way, retVal will become true (found a valid item), so it should stay true. Of course, a sane person would just say "if(item.isValid) return true;" but that's neither here nor there.

Also my favorite is still

code:

#define private public
#define protected public

# ? Mar 21, 2008 20:12

hey mom its 420: May 12, 2007

Oh yeah, drat, missed that.
That's exactly why you shouldn't try to be clever in your code and just use the most readable and clear code possible (in this case, an if statement)

# ? Mar 21, 2008 20:15

ehnus: Apr 16, 2003; Now you're thinking with portals!

Avenging Dentist posted:

Also my favorite is still
code:
#define private public
#define protected public

I've done that occasionally for writing test code (along with "#define class struct" :shobon:

) but, yeah, finding it in something that's about to ship always amusing.

# ? Mar 21, 2008 20:22

dwazegek: Feb 11, 2005; WE CAN USE THIS

Bonus posted:

Also what the hell is up with setting a variable to false and then doing var = var || x

So that if you find one true item, the true value is retained throughout the rest of the loop, and because retVal is true, item.IsValid is no longer evaluated.

Anyway, those two examples are pretty old, probably from when he first started working here, and seeing as I cringe at my old code as well, I can't really blame him. I found this abortion in a project I wrote about a year ago (and even then, I should have known better):

code:

public static void main(string[] args)
{
  //first argument passed is a Url, if a second and third argument are passed,
  //then they're considered to be the user name and password to use for authentication
  String url = args[0];
  String userName = null;
  String password = null;
  try
  {
    userName = args[1];
    password = args[2];
  }
  catch(ArrayIndexOutOfBoundsException ex) { }
  
  Open(url, userName, password); 
}

# ? Mar 21, 2008 20:24

npe: Oct 15, 2004

I've got two PL/SQL ones that I found recently. First, I found this:

code:


        EXCEPTION WHEN OTHERS THEN
            io_jobid := NULL;
            COMMIT;
        END;

For those of you who don't know PL/SQL but maybe know java, EXCEPTION WHEN OTHERS is the equivalent of "catch (Exception e)", which is bad enough. Putting the commit in there is like adding an additional "gently caress you" to whoever is calling the code... swallowing your exception and then preventing you from a rollback. :gonk:

Also, this gem was found by a coworker and while it doesn't look terrible, it led to a ridiculous bug:

code:

CREATE OR REPLACE PROCEDURE delete_user

<snip>

    UPDATE users
       SET is_deleted = 1
     WHERE userid = delete_user.userid;

    COMMIT;
END;

Inside this code is lurking an insidious bug: at some point the "users" table was altered and the column "userid" went away (replaced by another column). This package should have been changed but was not... Normally in Oracle when a table is altered, any packages that reference that table are invalidated and have to be recompiled (which happened), and if there are references to non-existent columns the compile will fail. But this code compiled just fine, and it even runs!

Of course, since userid can't be found in the table, and it wasn't explicitly referenced as users.userid, Oracle is helpful and assumes you mean the lexically scoped delete_user.userid, turning that condition into WHERE delete_users.userid = delete_users.userid. So we've set is_deleted on every single user!

And then it loving commits!

Jesus gently caress people, do not ever put commit in your procedure.

npe fucked around with this message at 21:52 on Mar 21, 2008

# ? Mar 21, 2008 20:54

The Oid: Jul 15, 2004; Chibber of worlds

Avenging Dentist posted:

Also my favorite is still
code:
#define private public
#define protected public

Haha, that reminds me of some code that was found in a header of some middleware, which shall remain nameless

code:

#ifndef TRUE
#define TRUE true
#endif

#ifndef FALSE
#define FALSE true
#endif

# ? Mar 21, 2008 20:58

_aaron: Jul 24, 2007; The underscore is silent.

code:

#ifndef TRUE
#define TRUE true
#endif

#ifndef FALSE
#define FALSE true
#endif

Good loving god! What could the purpose of this possibly be?

I remember a long time ago seeing something similar on theDailyWTF...

code:

#define true 0
#define false 1

Can you imagine trying to debug an application written this way?

# ? Mar 21, 2008 21:20

return0: Apr 11, 2007

While I've got nothing as bad as that which has been posted, a colleague of mine was looking through some of my code and found something equivalent to this gem:

code:

private void SomeFunc(double _a, double _b)
{
    if(_a / _b > 1)
    {
        //...do stuff...
    }
    else
    {
        //...do other stuff
    }
}

I have absolutely no idea what the hell I was thinking. It did get written on a whiteboard under the heading "return0's FUCKUPS". Thankfully that's the only entry (so far). I've also spent five minutes trying to figure out why something I was scaling by zero wasn't visible. No scaling needed so scale by zero am I right?!

# ? Mar 21, 2008 21:22

floWenoL: Oct 23, 2002

Avenging Dentist posted:

Some point along the way, retVal will become true (found a valid item), so it should stay true. Of course, a sane person would just say "if(item.isValid) return true;" but that's neither here nor there.

Also my favorite is still
code:
#define private public
#define protected public

code:

#define private public
#define protected public
#define class struct

# ? Mar 21, 2008 21:39

wwb: Aug 17, 2004

csammis posted:

This would be perfectly fine if he had ever heard of the "break" keyword

True that. Why all the hate for "single point of return"? I don't follow it 100%, but it can definitely make sense in some scenarios.

My favorite WTF come from our HTML types who don't get simple concepts like "don't repeat yourself", "non-destructive testing" and "find and replace is not a valid maintenance technique."

# ? Mar 22, 2008 00:59

floWenoL: Oct 23, 2002

wwb posted:

True that. Why all the hate for "single point of return"? I don't follow it 100%, but it can definitely make sense in some scenarios.

Because it's an archaic overreaction to spaghetti code. It should be used only when doing so would make the code most clearly understandable.

# ? Mar 22, 2008 01:21

dwazegek: Feb 11, 2005; WE CAN USE THIS

wwb posted:

True that. Why all the hate for "single point of return"? I don't follow it 100%, but it can definitely make sense in some scenarios.

It really depends on the circumstances, but I've seen situations where it just looks like he's jumping through hoops in order to maintain that pattern, instead of writing code readable code.

One of my gripes with it is the "retVal" variable (it always seems to be named that way), in the vast majority of the cases, the first "real" value that is assigned to it, is the value that it's going to have at the end of the method. So why not return the value immediately?

Personally, I prefer returning the value immediately, because it makes reading the code easier (or at least, easier for me); if I see a branching statement that immediately returns a value, then I know that the method is done if that branch is hit. If you go with the "single point of return" method, then this is less clear.

# ? Mar 22, 2008 01:44

Incoherence: May 22, 2004; POYO AND TEAR

floWenoL posted:

Because it's an archaic overreaction to spaghetti code. It should be used only when doing so would make the code most clearly understandable.

But at the same time, not using it ever is an overreaction. If it makes sense, use it. If it doesn't, don't use it.

dwazegek posted:

the "retVal" variable (it always seems to be named that way)

Personally when I write a IsFooCondition() that calls for such a variable, I feel stupid calling the variable some variant of isFooCondition.

Incoherence fucked around with this message at 01:51 on Mar 22, 2008

# ? Mar 22, 2008 01:49

dwazegek: Feb 11, 2005; WE CAN USE THIS

Incoherence posted:

But at the same time, not using it ever is an overreaction. If it makes sense, use it. If it doesn't, don't use it.

I don't think anyone will argue with that, my annoyance with it is that my colleague always uses it, sometimes to the point of absurdity.

Imagine methods with 10+ levels of indentation and stuff like this appearing regularly:

code:

Incoherence posted:

Personally when I write a IsFooCondition() that calls for such a variable, I feel stupid calling the variable some variant of isFooCondition.

I'm partial to the almost always applicable "result" :downs:

dwazegek fucked around with this message at 01:56 on Mar 22, 2008

# ? Mar 22, 2008 01:54

tef: May 30, 2004; -> some l-system crap ->

dwazegek posted:

I'm partial to the almost always applicable "result"

See, where you might use "result" I would use bln_flg_tst_cnd_args_x.

# ? Mar 22, 2008 02:01

POKEMAN SAM: Jul 8, 2004

tef posted:

See, where you might use "result" I would use bln_flg_tst_cnd_args_x.

Hahahahahahahahahahaha

Oh COBOL, you humor me so.

# ? Mar 22, 2008 02:48

Incoherence: May 22, 2004; POYO AND TEAR

tef posted:

See, where you might use "result" I would use bln_flg_tst_cnd_args_x.

And CoC has its first in-joke.

# ? Mar 22, 2008 02:49

such a nice boy: Mar 22, 2002

return0 posted:

While I've got nothing as bad as that which has been posted, a colleague of mine was looking through some of my code and found something equivalent to this gem:
code:
private void SomeFunc(double _a, double _b)
{
    if(_a / _b > 1)
    {
        //...do stuff...
    }
    else
    {
        //...do other stuff
    }
}

What's wrong with that code? Is it just that

code:

if (_a > _b)

would have been better?

# ? Mar 22, 2008 04:03

ShoulderDaemon: Oct 9, 2003; support goon fund; Taco Defender

such a nice boy posted:

What's wrong with that code? Is it just that
code:
if (_a > _b)
would have been better?

That's not exactly the same... he would need something like

code:

(abs(_a) > abs(_b)) && (sign(_a) == sign(_b))

although your code would work for the unsigned case.

# ? Mar 22, 2008 04:19

Vulture Culture: Jul 14, 2003; I was never enjoying it. I only eat it for the nutrients.

Never has a single empty method call made me want to hit something so badly.

code:

$self->maybeTakeUserFromForm();

# ? Mar 22, 2008 06:08

Victor: Jun 18, 2004

tef posted:

See, where you might use "result" I would use bln_flg_tst_cnd_args_x.

You almost made me spit coffee over my new, broken-in MS 4000 keyboard!

# ? Mar 22, 2008 06:16

1337JiveTurkey: Feb 17, 2005

dwazegek posted:

One of my gripes with it is the "retVal" variable (it always seems to be named that way), in the vast majority of the cases, the first "real" value that is assigned to it, is the value that it's going to have at the end of the method. So why not return the value immediately?

Personally, I prefer returning the value immediately, because it makes reading the code easier (or at least, easier for me); if I see a branching statement that immediately returns a value, then I know that the method is done if that branch is hit. If you go with the "single point of return" method, then this is less clear.

RetVal is useful if you're caching outputs somehow like in a map. Since many cache implementations can asynchronously dump some data between testing for its presence and actually getting the data itself, a common idiom is to get the item in question and then test for nullity. If it's there, we got a value. If it's not, we got null and need to switch to Plan B.

# ? Mar 22, 2008 06:32

duck monster: Dec 15, 2004

Mr. Heavy posted:

Never has a single empty method call made me want to hit something so badly.
code:
$self->maybeTakeUserFromForm();

Fuzzy logic!

# ? Mar 22, 2008 06:49

Adbot: ADBOT LOVES YOU

# ? Apr 25, 2024 13:53

hey mom its 420: May 12, 2007

Mr. Heavy posted:

Never has a single empty method call made me want to hit something so badly.
code:
$self->maybeTakeUserFromForm();

I see someone has been reading Schr�dinger's book on programming.

# ? Mar 22, 2008 13:25

The Something Awful Forums > Discussion > Serious Hardware/Software Crap > The Cavern of COBOL > Coding Horrors: You can gather all your technical debt into one easy framework!

«‹›1501 »