The Nightstar Zoo

This thread is for posting Appalling Code Written By Other Coders.

Include a comment if you want to point out what specifically makes you crazy about a bit of code.

Bonus points if the code (a) works anyway or (b) got checked into master sources yet doesn't even compile. (If you post code that somehow does both, YOU WIN! (I have no idea what.))

I'll start. This from the contractor who just got fired:





Thank you in advance for not posting everything I have ever written to this thread...

Don't know who. Don't know why. Just know that it's in some test code I have to work on.

Fut the Whuck!?

Vorn

Same project, different coder:

Our UI has buttons down the right-hand side of the screen for easy access on a touch-LCD screen. Thing is, some users are left-handed. Okay, we can swap the buttons, right?



That's the second time I've seen that kind of thing buried in release code....

sniff sniff It smells of generated code. Are you sure this was hand-written?

Yes. It may by failing your Turing Test because it was written by a programmer with 2 months of experience; much of that 2 months' experience was spent overcoming cultural and language barriers. He's an H1B worker who speaks English as a second language.

Ran across this snippet today. Apparently I do, indeed, work with idiots.


If I could find the person responsible, I'd probably suggest that he rewrite the code so that it's cleaner.
But then again, he probably wouldn't get the joke.

Here's my contribution on the topic of Appalling Code By Other Coders:



No offense to the Nightstar Coding Team, they were handed absolute crap to begin with, and they're doing the best they can with what they've got.


And my second contribution:

_________________
Fandemonium 2010 -- No Boundaries.
http://www.fandemonium.org
Friday - Sunday, August 6th - 8th, 2010
Nampa Civic Center - Nampa, Idaho (Only 20 minutes from the airport!)
(Idaho: It ain't just potatoes anymore.)

It's all commenting... what's that got to do with the code (which isn't shown)?

As long as audio, video, and subpicture are the only tests, you could do


or, you could wrap that in the above



These may not be applicable in your code though, you probably heavily sanitized it.

I had started to post something like this myself, when I noticed the last line of Pi's posting:



So I think Pi knew that already.

Kaz didn't get the joke either. *flrrd*

I did get it, although I was thinking that his actual code was more complex than what he posted. Notice the "You probably heavily sanitized it"?

When I learned COM, I don't remember them saying anything about summing error codes.

While it is lousy design, it would work, if all you care about is whether one of the tests passed. It works because in C, any value other than 0 is considered true.

Mind you, it probably does not work as it was intended; in most cases, it would make more sense to test whether one or more of the tests failed, especially since this version doesn't tell you which test passed (whereas if you were testing for a single point of failure, the specific failed function may not be important).

Even if you were only seeking a single working function, this version is wasteful because it keeps testing the functions even after a positive match has been found.

Assuming it is in fact written as intended, it would probably make more sense to write it like this:



Though that assumes that you can freely change the return type of the function..
Comments and corrections welcome.

Oh this is just too much. There are some problems in that, SRL.

The *correct* way to write it is the following:

The problem with your version is that it can be 0 on success (NULL) and 0 on failure (i). Not good. Besides that you should use the mechanisms you're given instead of assuming things about the hidden data types. Finally your code is far less readable than the above, and expanded unnecessarily. The problem was never the performance.

Good point, yes, I rather dropped the ball there.

This code registers an interface with the Global Interface Table, and adopts the interface (increments its refcount) so that the caller who created it can just throw his copy away. Everyone, including the caller, will get this interface back out of the GIT later.

That's just background. The thing that made me splutter is on line 4, after the comment.

Please hurt the author of that bit of code for me...thank you.

Trust me, if he still had a job here, I would have.

Oh, how I wish the apps I inherited from my predecessor weren't giving me so much fodder to post in this thread.

Wha...?

What would that even do?

Apparently it tries to release the draw thing. If it fails, it tries again.

I wonder how much processor that thing takes up when it runs, if it can't release.

Vorn

COM interfaces are refcounted, and the refcount is managed through IUnknown::AddRef() and IUnknown::Release(). (All COM interfaces inherit from IUnknown).

AddRef() increments the refcount. Release() decrements the refcount. Both return the new count. A Release() call that decrements the count to zero also causes the object to delete itself.

There are reasons that you might do this (most notably, to prevent an object leak when shutting down the component), but all of them are bad reasons, and none is an excuse for not calling Release() exactly once per pointer, by the component that performed the AddRef().

This is also why smart pointers are such a wonderful creation.

You'd love objectstore.

We instantiate 4 objects of this class. Each object must be threadsafe.





In a perverse sort of way, I am attracted to this at the same time that it makes me splutter. Static variables inside class methods act like instance variables, except that their scope is limited to that particular method. Since this is only ever used in that one method, I could make some "good design" arguments about this.

However, it murders the standard idiom and violates the corpse in a manner too horrible to imagine.

This code has been in our codebase for three months without being noticed. Or, quite clearly, ever tested. The programmer that wrote it established a track record of cluelessness before getting himself fired, and today I decided to proactively code review some of his work. The funny thing about this is that it was the last line that caught my attention, which does nothing--it gets a pointer to the head of the m_vToolIdleTime vector, then throws it away because he doesn't do anything with it. I'm sure he thought he was changing an internal cursor to the vector or something. Normally this would make me ask "why?" but with this programmer I've learned that trying to answer that question just causes me to keen piteously and make sobbing noises.

It wasn't until I went back and examined the loop above that line that I stabbed my brain out.

What this is SUPPOSED to do, is read some data from a file, and create a new ToolIdleTimeInfo structure for each element. We push these into a vector. At some point we go back and calculate average tool utilization, which is a huge metric in the semiconductor industry. (This is a US$1.2M machine. It is understandable if management wants it to actually be in use for 90%+ of its uptime.)

What it actually DOES, however, is:

Create one ToolIdleTimeInfo structure, and get its pointer (which, as you may notice, is never tested to see if new succeeded). Load a record from file, write it to the memory pointed at by our pointer, and push the pointer into our vector. So far, so good. Now, load another record, and write it to the same location in memory. He failed to get another new pointer. This overwrites the previous record. Now, push the same pointer into the vector again.

End result, if you have n items in the data file, you will have a vector with n copies of the same pointer, all pointing to the piece of memory we started out with and which now contains the last record in the file.

Oh, and another missed test: he never truncates this vector, neither in the load, nor anywhere else at runtime. At runtime, he extends the vector correctly. Which means this program leaks about 100kB/hr. Given that this machine is expected to have uptimes of 2-6 months, they'd better have half a gigabyte of memory free just for this leaking vector.

Did I mention the customer signed off on this code, and we are now in "maintenance" mode?

This is shipped code.

Um... The bolded text above is patently wrong.

Whomever told you that was either kidding or really should be kept from writing OO software in C++. The code works correctly only if DoCallbackThingie is a static method or CMediatorCallback is a singleton. You are creating one static mutex to be shared by all of the code that use that function -- in particular, all four instances of CMediatorCallback (more precisely, all threads that uses that function, regardless of the value of the 0th parameter) are sharing one mutex.

In this particular case, the code is safe. The instances will block eachother out, but in doing so, they will certainly block out multiple threads on the same instance. The only penalty you incur from this particular error is one of performance -- Different instances of CMediatorCallback on different threads will be prevented from using DoCallbackThingie() at the same time when there is no good reason to block them.

There are at least three flavors of variables that can be declared with the 'static' keyword in C++, but no matter which one you use, the static variable always lives in the code segment, and is tied to the code that it appears near. You want something to exist on a per-object basis, it needs to live with the object pointer. That means putting it in the class declaration.

And really, would you want to deal with the maintenance nightmare that would inevitably come up if you could just ad-hoc add instance variables to a class by declaring them in the member functions?

BWAAAHAHAHAAAAA!

Okay, there's bad code and then there's evil code, and the difference is pretty much whether or not the code wears a stylish cape and has a secret lair.

This is a sanitized version of some bad code that made me splutter:



(I was reminded of this from a discussion with Pi, in which we discovered that it is possible to cast any integer to an enum, even if the value of the integer doesn't have a defined enum value.)

Now, I have no problem with quick shorthand initialization, EXCEPT when they are disparate types that happen to only share zero as a common "invalid value indicator".

It bothers me greatly that this code compiles without warnings.

Oddly enough, this code DOES NOT compile:



Because, while qaz will accept a 0, it won't accept QUX_NONE in an assignment. So, at least partial type safety is employed.

Anyway, long story short. I fixed the code. But you need to understand something: I'm a right bastard. Here is my replacement code. This program compiles and runs just duckily.