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!

«‹›1503 »

Dessert Rose: May 17, 2004; awoken in control of a lucid deep dream...

jarito posted:

Or just use a higher level construct like a List, Hash or Set.

You can go too far that way, though. I found this in a codebase I inherited:

code:

private Collection<Player> _players = new Collection<Player>();
        public ThisClass()
        {
            for (int i = 0; i < MaximumNumberOfPlayers; ++i)
            {
                _players.Add(null);
            }
        }

        public bool HasPlayer(int position)
        {
            return _players[position] != null;
        }

MaximumNumberOfPlayers is a constant.

Also, this object gets reused, and here's the method that gets called when that happens (and when it first gets set up, too)

code:

        public void ResetClass(long number)
        {
            _foo = new Foo(number);
            _players = new Collection<Player>();
        }

(and then _players is an empty collection, and it tries to index into it with HasPlayer during setup...)

I love broken code that looks like it works, but has enormous design flaws. I have spent the last two days of work on this project refactoring all the code surrounding this area.

And of course THIS part of the code has absolutely no unit tests whatsoever. Oh, and all the code is broken in subtle ways that are instantly apparent when I write the tests.

# ? Mar 29, 2010 20:59

Adbot: ADBOT LOVES YOU

# ? May 14, 2024 08:01

BigRedDot: Mar 6, 2008

The person that wrote this has been fired, sure, but that's meager consolation when I stumble across all the droppings he left. This was buried under a few levels of indentation, in the middle of a giant switch, in a 900 line function. I've preserved the formatting also for your enjoyment.

code:

    vector<Thing> filteredThings;
    filteredThings.clear();
    for(int i=0 ; i < someVector.size(); i++)
    {
        for(int j=0; j < mStupidNamingConvention.size(); j++ )
        {
            if((mStupidNamingConvention[j].GetAThing().GetValue("FooID")) == someVector[i].GetValue("FooID"))
            {
                filteredThings.push_back(someVector[i]);
                j = mStupidNamingConvention.size()+1;
            }
        }
    }

I think a break statement would have sufficed....

BigRedDot fucked around with this message at 00:33 on Mar 30, 2010

# ? Mar 29, 2010 21:40

Lexical Unit: Sep 16, 2003

code:

char* data = blarg ();
SomeShittyCppClass o;
unsigned len;
o.Parse (data, len);
f = fopen (o.GetValue ("fileName").c_str (), "wb");
wrote = fwrite ((char*)&o, len, 1, f);
assert (wrote == len);

Dude standing in my office just a few moments ago: "Why is my program asserting?!?" :qq:

# ? Mar 29, 2010 22:36

Painless: Jan 9, 2005; Turn ons: frogs, small mammals, piles of compost
Turn offs: large birds, pitchforks
See you at the beach!

That fwrite call is amazing.

# ? Mar 29, 2010 23:13

Flobbster: Feb 17, 2005; "Cadet Kirk, after the way you cheated on the Kobayashi Maru test I oughta punch you in tha face!"

I just wrote the programming equivalent of the "Buffalo buffalo buffalo" sentence:

code:

        Application application = (Application) Application.application();

The cast is necessary because the static method actually returns a reference to Application's base class (WOApplication), not our concrete subclass!

# ? Mar 30, 2010 18:53

deedee megadoodoo: Sep 28, 2000; Two roads diverged in a wood, and I, I took the one to Flavortown, and that has made all the difference.

Ooh! I get to contribute!

Came across this the other day while looking at a jsp that extracts the app version from pom.xml... it's always the first version node...

code:

    NodeList nodes = doc.getElementsByTagName("version");

    for (int i=0; i<nodes.getLength(); i++)
    {
        Node versionNode = nodes.item(i);

        // Use version node that is child of the doc root.
        if ( versionNode.getParentNode() == doc.getDocumentElement() )
        {
                buildInfo = versionNode.getFirstChild().getNodeValue();
                break;
        }
    }

Someone thought this was a good idea. I don't know why. I replaced the whole loop with this...

code:

    doc.getElementsByTagName("version").item(0).getFirstChild().getNodeValue();

The sad part is that I'm an SA. I don't even write code for a living.

deedee megadoodoo fucked around with this message at 19:28 on Mar 30, 2010

# ? Mar 30, 2010 19:26

blorpy: Jan 5, 2005

Lexical Unit posted:

code:

char* data = blarg ();

star on the left? thats teh real coding horror

# ? Mar 30, 2010 20:22

Shavnir: Apr 5, 2005; A MAN'S DREAM CAN NEVER DIE

Speaking of bad student code I remembered a real horror in Java from a ways back. It was for an app that had to read from a large database. Pretty simple, make a data storage object, get the cursor, do a for every and drop it into a generic list right?

Nope. For every column in the database they had an array. Its not like they didn't know about generics, there was one or two instances where a column had multiple values and ergo a LinkedList<String> but I swear the next time I see 40-50 sets of int[]s I'm going to drink until I don't remember it.

Oh and yes, each row in the database was a column in each array :suicide:

# ? Mar 30, 2010 20:36

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

Flobbster posted:

I just wrote the programming equivalent of the "Buffalo buffalo buffalo" sentence:
code:
        Application application = (Application) Application.application();
The cast is necessary because the static method actually returns a reference to Application's base class (WOApplication), not our concrete subclass!

This is wonderful.

# ? Mar 30, 2010 20:51

GROVER CURES HOUSE: Aug 26, 2007; Go on...

Flobbster posted:

I just wrote the programming equivalent of the "Buffalo buffalo buffalo" sentence:
code:
        Application application = (Application) Application.application();
The cast is necessary because the static method actually returns a reference to Application's base class (WOApplication), not our concrete subclass!

Code is Art.

# ? Mar 30, 2010 21:50

BigRedDot: Mar 6, 2008

More irredeemable code today (approximated):

code:

struct Thing {
   double foo;
   // ... 
};

class ThingContainer {
    int ContainerSize();
    string getID(int index);
    int GetFoo(int index);
    double GetBar(int index);

private:
   map<string, vector<Thing>> mIDToThings;
   map<string, vector<Thing>>::iterator ThingIt;

};

int ThingContainer::ContainerSize() 
{
    return (int)mIDToThings.size();
}

string ThingContainer::GetID(int index) 
{
   ThingIt=mIDToThings.begin();
   for(int i=0); i<index; i++)
      ThingIt++
   return ThingIt->first;
}

// This method was repeated verbatim for six or seven other fields in Thing
// It might be nice if the name mentioned something about computing an average
double ThingContainer::GetFoo(int index) 
{
   ThingIt=mIDToThings.begin();
   for(int i=0); i<index; i++)
      ThingIt++
   double sum = 0;
   for(int j=0; j<ThingIt->second.size(); j++)
   {
      sum+= ThingIt->second[j].foo;
   }
   return sum/ThingIt->second.size();
}

This was then used like:

code:

for(int i=0; i<mThingContainer.ContainerSize(); i++ )
{
    mTargetContainer.GetID(i); 
    mTargetContainer.GetFoo(i);  
    // six or seven more, one for each field in Thing
}

So yes, he's indexing a map like a vector and turning what should be a single log(n) lookup into a nice STL map into seven or eight linear lookups, every time. No, he never actually uses the map key except when iterating over the map by hand in a for loop.

BTW if you are a US citizen your tax dollars paid for this.

# ? Mar 31, 2010 20:40

HFX: Nov 29, 2004

BigRedDot posted:

More irredeemable code today (approximated):

code:

struct Thing {
   double foo;
   // ... 
};

class ThingContainer {
    int ContainerSize();
    string getID(int index);
    int GetFoo(int index);
    double GetBar(int index);

private:
   map<string, vector<Thing>> mIDToThings;
   map<string, vector<Thing>>::iterator ThingIt;

};

int ThingContainer::ContainerSize() 
{
    return (int)mIDToThings.size();
}

string ThingContainer::GetID(int index) 
{
   ThingIt=mIDToThings.begin();
   for(int i=0); i<index; i++)
      ThingIt++
   return ThingIt->first;
}

// This method was repeated verbatim for six or seven other fields in Thing
// It might be nice if the name mentioned something about computing an average
double ThingContainer::GetFoo(int index) 
{
   ThingIt=mIDToThings.begin();
   for(int i=0); i<index; i++)
      ThingIt++
   double sum = 0;
   for(int j=0; j<ThingIt->second.size(); j++)
   {
      sum+= ThingIt->second[j].foo;
   }
   return sum/ThingIt->second.size();
}

This was then used like:

code:

for(int i=0; i<mThingContainer.ContainerSize(); i++ )
{
    mTargetContainer.GetID(i); 
    mTargetContainer.GetFoo(i);  
    // six or seven more, one for each field in Thing
}

Wouldn't surprise me in the least. Fortunately, I know corporations do no better. I've just got finished cleaning up some javascript where the same 12 lines were posted 9 times in the same file.

# ? Mar 31, 2010 20:49

Shumagorath: Jun 6, 2001

dwazegek posted:

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

I remember the day I found out that was a Pascal keyword... the hard way.

Dijkstracula posted:

God drat, that's one smarmy article.

M$$$

GNU is basically the worst anything and the coding style only affirms this.

Shumagorath fucked around with this message at 02:58 on Apr 2, 2010

# ? Apr 2, 2010 01:38

trex eaterofcadrs: Jun 17, 2005; My lack of understanding is only exceeded by my lack of concern.

Hey I've got an idea, let's make our urls seo friendly!
:allears:

OOh what a good idea! But wait, we didn't use the link function for all our href's and I don't wanna go back and edit my poo poo spaghetti code!
:downs:

No I got this, see if we capture all the output and then push it through this:

code:

function callback($pagecontent) {
  // find all the href thatr are part of the <a> tag
  $pagecontent = preg_replace_callback("/(<a\s+[^>]*href=['\"]{1})([^'\">]+)([^>]*>)/", 'transform_uri', $pagecontent);

  return $pagecontent;
}

then no one has to do anything!
:allears:

Sounds good to me! I'd hate to have to fix my poo poo!

...Later...

:mad:

WHAT THE gently caress WHY IS THIS THING NOT REWRITING MY FORM ACTION URLS. *looks at seo code*
:mad:

you motherfuckers

# ? Apr 3, 2010 19:56

Kidane: Dec 15, 2004; DANGER TO MANIFOLD

I always read all the 'preg' functions in PHP as 'pregnant'.

# ? Apr 5, 2010 21:47

Jonnty: Aug 2, 2007; The enemy has become a flaming star!

Kidane posted:

I always read all the 'preg' functions in PHP as 'pregnant'.

I've barely used PHP and always find that funny. What does it actually stand for?

# ? Apr 5, 2010 22:06

pokeyman: Nov 26, 2006; That elephant ate my entire platoon.

Perl-compatible regular expressions.

# ? Apr 5, 2010 22:22

bitreaper: Jan 1, 2007

I thought it was Posix REGular expressions, and pcre was the perl ones.

# ? Apr 6, 2010 20:41

musclecoder: Oct 23, 2006; I'm all about meeting girls. I'm all about meeting guys.

bitreaper posted:

I thought it was Posix REGular expressions, and pcre was the perl ones.

Nope, ereg() was the Posix regular expression function and preg_* are the PCRE methods. Of course, you shouldn't be using ereg() or any of the variants.

# ? Apr 6, 2010 20:50

Meganiuma: Aug 26, 2003

pseudorandom name posted:

The deadlock detection probably isn't smart enough to deal with the ordering.

Edit: What OS is this from? My quick poll of an IRC channel filled with devs familiar with Linux, *BSD, VxWorks and Solaris produced no answer. The consensus guess was Mach and/or OS X.

Talk about a late reply. I don't come to the forums that often. It's not an OS. It's RDBMS kernel code that I've obfuscated to protect the guilty. and no,the deadlock detector should be sufficient to catch any true deadlock (i.e. multiple processes blocking with a cycle in the 'waits for' graph); that loop would only make sense if it would be possible for the deadlock detector to give false positives due to ordering issues, which I believe it can't.

Meganiuma fucked around with this message at 18:13 on Apr 7, 2010

# ? Apr 7, 2010 18:03

A A 2 3 5 8 K: Nov 24, 2003; Illiteracy... what does that word even mean?

quote:

@mtabini: Once you learn how powerful arrays in PHP are, you realize that every other language must, in a sense, be kidding.

I must be unenlightened because http://us2.php.net/manual/en/ref.array.php makes me want to throw up.

# ? Apr 8, 2010 07:21

Shumagorath: Jun 6, 2001

A A 2 3 5 8 K posted:

I must be unenlightened because http://us2.php.net/manual/en/ref.array.php makes me want to throw up.

I must be in the dark too since that just looks like a list of helper functions that most competent C programmers could implement on demand. How does that make the data structure "more powerful"?

# ? Apr 8, 2010 08:09

Scaevolus: Apr 16, 2007

PHP arrays: because every other data structure in the standard library is even worse.

# ? Apr 8, 2010 11:15

Internet Janitor: May 17, 2008; "That isn't the appropriate trash receptacle."

php.net - next() posted:

Returns the array value in the next place that's pointed to by the internal array pointer, or FALSE if there are no more elements.

Warning:

This function may return Boolean FALSE, but may also return a non-Boolean value which evaluates to FALSE, such as 0 or "". Please read the section on Booleans for more information. Use the === operator for testing the return value of this function.

Oh, PHP.

# ? Apr 8, 2010 12:00

markerstore: Dec 5, 2003; Canny!

Internet Janitor posted:

Oh, PHP.

What if the next element in the array is FALSE?

# ? Apr 8, 2010 15:56

Athas: Aug 6, 2007; fuck that joker

Internet Janitor posted:

Oh, PHP.

What if the next element in the array is the boolean value FALSE?

Edit: :arghfist:

Athas fucked around with this message at 16:03 on Apr 8, 2010

# ? Apr 8, 2010 16:01

Blotto Skorzany: Nov 7, 2008; He's a PSoC, loose and runnin'
came the whisper from each lip
And he's here to do some business with
the bad ADC on his chip
bad ADC on his chiiiiip

Scaevolus posted:

PHP arrays: because every other data structure in the standard library is even worse.

Does PHP's core or standard library have any other data structures besides its array/hash mashups?

# ? Apr 8, 2010 18:24

king_kilr: May 25, 2007

Otto Skorzeny posted:

Does PHP's core or standard library have any other data structures besides its array/hash mashups?

Depends how you define "standard library" do you mean the unholy mess of everything they just threw in the global namespace, or the *actual* SPL. I think the SPL has some more datastructures, they probably suck though.

# ? Apr 8, 2010 18:30

MononcQc: May 29, 2007

Otto Skorzeny posted:

Does PHP's core or standard library have any other data structures besides its array/hash mashups?

yes: doubly linked lists, stacks, queues, heaps, nax/min heaps, priority queues and fixed arrays. I've never really seen them used though.

# ? Apr 8, 2010 18:33

Captain Capacitor: Jan 21, 2008; The code you say?

And let's not forget the myriad of frameworks that create their own versions of certain datatypes.

# ? Apr 8, 2010 19:15

Wheany: Mar 17, 2006; Spinya^{ha^{ha^haha}ha}ha_{ha_{ha_haha}ha}ha!; Doctor Rope

markerstore posted:

What if the next element in the array is FALSE?

Then the function throws a EverytingIsFine exception, which evaluates to FALSE.

# ? Apr 8, 2010 20:56

Shavnir: Apr 5, 2005; A MAN'S DREAM CAN NEVER DIE

markerstore posted:

What if the next element in the array is FALSE?

the horror posted:

Note: You won't be able to distinguish the end of an array from a boolean FALSE element. To properly traverse an array which may contain FALSE elements, see the each() function.

# ? Apr 8, 2010 21:20

Arms_Akimbo: Sep 29, 2006; It's so damn...literal.

I started a new job a few months ago. My main responsibility is maintaining a nine year old application that has had probably half a dozen mostly unsupervised developers work on it in the past, so its a giant mess. One dev would have so many nested ifs that she would block quote her ends like she was leaving herself breadcrumbs. So not only is the code littered in {x}s, but it does a fantastic job of screwing up the process of commenting things out.

Thankfully, the code is also good for a laugh every now and then.

code:

bFoo := True;
if bFoo = True then begin
  // something
end;

I guess whoever wrote that didn't trust delphi's ability to assign values. They were also big fans of Hungarian notation, but could never be bothered to fix their variables if they decided they had to change the type, so there's lots of

code:

sFoo:  integer;
iBar:  boolean;

The icing on the cake has to be that every single procedure or function wants a Sender: TObject as its first parameter, even though a majority of them aren't even related to an object, so there's foo(nil, bar) all over the place.

# ? Apr 8, 2010 21:32

NotShadowStar: Sep 20, 2000

A A 2 3 5 8 K posted:

I must be unenlightened because http://us2.php.net/manual/en/ref.array.php makes me want to throw up.

That's pretty funny because the last time I had to do something semi-complicated with PHP arrays I pretty much had to implement a sorting function using the usort function listed there.

:php:

# ? Apr 8, 2010 22:49

evensevenone: May 12, 2001; Glass is a solid.

Arms_Akimbo posted:

So not only is the code littered in {x}s, but it does a fantastic job of screwing up the process of commenting things out.

I think I've found your answer:

quote:

Thankfully, the code is also good for a laugh every now and then.
code:
bFoo := True;
if bFoo = True then begin
  // something
end;

Why comment things out when you can set a bool to false?

# ? Apr 9, 2010 01:06

TOO SCSI FOR MY CAT: Oct 12, 2008; this is what happens when you take UI design away from engineers and give it to a bunch of hipster art student "designers"

This is what happens when you tell a bunch of Windows developers to write a Linux application:

code:

void TryToRemoveFile(const std::string& filename) {
    if (FileExists(filename)) {
        unlink(filename.c_str());
    }
}

# ? Apr 9, 2010 19:07

Shumagorath: Jun 6, 2001

Janin posted:

This is what happens when you tell a bunch of Windows developers to write a Linux application:
code:
void TryToRemoveFile(const std::string& filename) {
    if (FileExists(filename)) {
        unlink(filename.c_str());
    }
}

Developing for Windows destroys their ability to read APIs and/or understand how a filesystem works? Am I missing something about NTFS?

# ? Apr 9, 2010 19:14

TOO SCSI FOR MY CAT: Oct 12, 2008; this is what happens when you take UI design away from engineers and give it to a bunch of hipster art student "designers"

Shumagorath posted:

Developing for Windows destroys their ability to read APIs and/or understand how a filesystem works? Am I missing something about NTFS?

They wrap every "cryptic" function with a "properly named" one, usually without bothering to figure out the exact behavior of the function being wrapped:

unlink -> TryToRemoveFile
rand -> GenerateRandomNumber, GenerateRandomNumberEx
stat -> ReadFileInfo, ReadFileInfoW

etc

# ? Apr 9, 2010 23:03

Dessert Rose: May 17, 2004; awoken in control of a lucid deep dream...

Janin posted:

They wrap every "cryptic" function with a "properly named" one, usually without bothering to figure out the exact behavior of the function being wrapped:

unlink -> TryToRemoveFile
rand -> GenerateRandomNumber, GenerateRandomNumberEx
stat -> ReadFileInfo, ReadFileInfoW

etc

haha, they seriously try to emulate the *Ex naming style from Win32? :bang:

# ? Apr 9, 2010 23:07

Adbot: ADBOT LOVES YOU

# ? May 14, 2024 08:01

Blotto Skorzany: Nov 7, 2008; He's a PSoC, loose and runnin'
came the whisper from each lip
And he's here to do some business with
the bad ADC on his chip
bad ADC on his chiiiiip

code:

sub dumphash {
   my $hash = pop;
   my $key, $down;
   for $key (keys %$hash) {
      if ($key eq "result") {
         $down = $hash->{$key};
         for $key (keys %$down) {
            print "result ==> $key => $down->{$key}\n";
         }
      }
   }
}

# ? Apr 9, 2010 23:30

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!

«‹›1503 »