The Nightstar Zoo

Nightstar IRC Network - irc.nightstar.net
It is currently Fri May 26, 2017 12:00 pm

All times are UTC - 6 hours [ DST ]




Post new topic Reply to topic  [ 115 posts ]  Go to page 1, 2, 3, 4  Next
Author Message
PostPosted: Tue Aug 31, 2004 6:03 pm 
Offline
Nightstar Graveyard Daemon
User avatar

Joined: Mon Jun 03, 2002 8:30 pm
Posts: 1071
Location: Wouldn't you rather observe my Velocity?
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:

Code:
IPMControlPtr pPMInterface = (*itPM).GetPMInterface();

// Apparently they don't have -> in the country where he's from.




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


Top
 Profile  
 
 Post subject:
PostPosted: Fri Sep 03, 2004 6:55 pm 
Don't know who. Don't know why. Just know that it's in some test code I have to work on.
Code:
TestCategory CDVDTestCases::GetCategory()
{
    if (All == m_CurrentCategory)
    {
        m_CurrentCategory = All;
    }
    return m_CurrentCategory;
}


Top
  
 
 Post subject:
PostPosted: Fri Sep 03, 2004 7:21 pm 
Offline
Energizer Bunny
User avatar

Joined: Wed May 22, 2002 12:24 am
Posts: 1634
Fut the Whuck!?

Vorn


Top
 Profile  
 
 Post subject:
PostPosted: Sat Sep 04, 2004 1:09 am 
Offline
Nightstar Graveyard Daemon
User avatar

Joined: Mon Jun 03, 2002 8:30 pm
Posts: 1071
Location: Wouldn't you rather observe my Velocity?
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?

Code:
if( lSide != m_currentSide )
    MoveAllButtons( lSide );
else
    MoveAllButtons( lSide );


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


Top
 Profile  
 
 Post subject:
PostPosted: Tue Sep 14, 2004 1:46 am 
sniff sniff It smells of generated code. Are you sure this was hand-written?


Top
  
 
 Post subject:
PostPosted: Tue Sep 14, 2004 5:46 pm 
Offline
Nightstar Graveyard Daemon
User avatar

Joined: Mon Jun 03, 2002 8:30 pm
Posts: 1071
Location: Wouldn't you rather observe my Velocity?
Schol-R-LEA wrote:
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.


Top
 Profile  
 
 Post subject:
PostPosted: Fri Oct 01, 2004 4:02 pm 
Ran across this snippet today. Apparently I do, indeed, work with idiots.
Code:
if (Audio == nextTest) {
  currentTest = Audio;
}
else if (Video == nextTest) {
  currentTest = Video;
}
else if (Subpicture == nextTest) {
  currentTest = Subpicture;
}


If I could find the person responsible, I'd probably suggest that he rewrite the code so that it's cleaner.
Code:
switch(nextTest) {
  case Audio: currentTest = Audio; break;
  case Video: currentTest = Video; break;
  case Subpicture: currentTest = Subpicture; break;
}

But then again, he probably wouldn't get the joke.


Top
  
 
 Post subject:
PostPosted: Sun Oct 03, 2004 4:06 pm 
Offline
Knight of Daisies, Tulip Slayer
User avatar

Joined: Sat May 11, 2002 3:03 am
Posts: 1621
Location: Sector ZZ9 Plural Z Alpha
Here's my contribution on the topic of Appalling Code By Other Coders:

Code:
/************************************************************************
 *   IRC - Internet Relay Chat, src/nightstar.c
 *
 *   This program is free software; you can redistribute it and/or modify
 *   it under the terms of the GNU General Public License as published by
 *   the Free Software Foundation; either version 1, or (at your option)
 *   any later version.
 *
 *   This program is distributed in the hope that it will be useful,
 *   but WITHOUT ANY WARRANTY; without even the implied warranty of
 *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 *   GNU General Public License for more details.
 *
 *   You should have received a copy of the GNU General Public License
 *   along with this program; if not, write to the Free Software
 *   Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
 */

#ifdef lint
static char sccsid[] = "@(#)nightstar.c     8.00 10/13/02 NightstarIRCd ";
#endif


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:

Code:
<?php
/***************************************************************
*  Copyright notice

*  (c) 1999-2003 Kasper Skårhøj (kasper@typo3.com)
*  All rights reserved
*
*  This script is part of the Typo3 project. The Typo3 project is
*  free software; you can redistribute it and/or modify
*  it under the terms of the GNU General Public License as published by
*  the Free Software Foundation; either version 2 of the License, or
*  (at your option) any later version.
*
*  The GNU General Public License can be found at
*  http://www.gnu.org/copyleft/gpl.html.
*  A copy is found in the textfile GPL.txt and important notices to the license
*  from the author is found in LICENSE.txt distributed with these scripts.
*
*
*  This script is distributed in the hope that it will be useful,
*  but WITHOUT ANY WARRANTY; without even the implied warranty of
*  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
*  GNU General Public License for more details.
*
*  This copyright notice MUST APPEAR in all copies of the script!
***************************************************************/
/**
 * This is the MAIN DOCUMENT of the TypoScript driven standard front-end

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


Top
 Profile  
 
 Post subject:
PostPosted: Sun Oct 03, 2004 7:26 pm 
It's all commenting... what's that got to do with the code (which isn't shown)? :P


Top
  
 
 Post subject:
PostPosted: Mon Oct 04, 2004 2:55 pm 
Pi wrote:
Ran across this snippet today. Apparently I do, indeed, work with idiots.
Code:
if (Audio == nextTest) {
  currentTest = Audio;
}
else if (Video == nextTest) {
  currentTest = Video;
}
else if (Subpicture == nextTest) {
  currentTest = Subpicture;
}


If I could find the person responsible, I'd probably suggest that he rewrite the code so that it's cleaner.
Code:
switch(nextTest) {
  case Audio: currentTest = Audio; break;
  case Video: currentTest = Video; break;
  case Subpicture: currentTest = Subpicture; break;
}

But then again, he probably wouldn't get the joke.


As long as audio, video, and subpicture are the only tests, you could do
Code:
currentTest = nextTest;


or, you could wrap that in the above

Code:
switch(nextTest) {
  case Audio: case Video: case Subpicture:
        currentTest = nextTest; break;
}


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


Top
  
 
 Post subject:
PostPosted: Mon Oct 04, 2004 3:19 pm 
I had started to post something like this myself, when I noticed the last line of Pi's posting:

Pi wrote:
But then again, he probably wouldn't get the joke.


So I think Pi knew that already.


Top
  
 
 Post subject:
PostPosted: Thu Oct 07, 2004 11:40 am 
Kaz didn't get the joke either. *flrrd*


Top
  
 
 Post subject:
PostPosted: Thu Oct 07, 2004 11:42 pm 
Raif wrote:
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"?


Top
  
 
 Post subject:
PostPosted: Fri Oct 29, 2004 6:17 pm 
Code:
HRESULT hr = 0;
for (i = 0; i < NUMCASES; i++)
  hr += pComInterface->FunctionToTest(i);
return (hr == 0) ? PASSED : FAILED;


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


Top
  
 
 Post subject:
PostPosted: Sat Oct 30, 2004 1:17 pm 
Pi wrote:
Code:
HRESULT hr = 0;
for (i = 0; i < NUMCASES; i++)
  hr += pComInterface->FunctionToTest(i);
return (hr == 0) ? PASSED : FAILED;


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:

Code:
HRESULT hr = 0;

for (i = 0; i < NUMCASES; i++)
{
    hr = pComInterface->FunctionToTest(i);
     if (hr)
        break;
}

/* return the index of the function, or else NULL
return ((hr) ? i : NULL);


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


Last edited by Schol-R-LEA on Sat Oct 30, 2004 4:03 pm, edited 1 time in total.

Top
  
 
 Post subject:
PostPosted: Sat Oct 30, 2004 2:35 pm 
Oh this is just too much. There are some problems in that, SRL.

The *correct* way to write it is the following:
Code:
for (i = 0; i < NUMCASES; i++)
  if (S_OK != pComInterface->FunctionToTest(i)) return FAILED;
return PASSED;

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.


Top
  
 
 Post subject:
PostPosted: Sat Oct 30, 2004 4:05 pm 
Good point, yes, I rather dropped the ball there.


Top
  
 
 Post subject:
PostPosted: Tue Nov 02, 2004 1:43 pm 
Offline
Nightstar Graveyard Daemon
User avatar

Joined: Mon Jun 03, 2002 8:30 pm
Posts: 1071
Location: Wouldn't you rather observe my Velocity?
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.

Code:
      HRESULT hr = m_pGIT->RegisterInterfaceInGlobal(pPMCB, IID_IPMControlCB, &dwCookie);
      if( SUCCEEDED(hr) ) {
         // cache this interface, so we can hold its refCount up (caller can then release it)
         m_PmCBCookies[dwCookie] = pPMCB.operator =;
         m_PmCBCookies[dwCookie]->AddRef();
         m_StationCookiesCB[nStationID] = dwCookie;
      } else {


Top
 Profile  
 
 Post subject:
PostPosted: Wed Nov 03, 2004 8:41 pm 
Code:
int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine, int nCmdShow )
{
   MSG         msg;
   int         iDeviceNum = 1;
   
   hPrevInstance = hPrevInstance;
   lpCmdLine = lpCmdLine;


Top
  
 
 Post subject:
PostPosted: Fri Nov 05, 2004 8:05 pm 
Pi wrote:
Code:
int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine, int nCmdShow )
{
   MSG         msg;
   int         iDeviceNum = 1;
   
   hPrevInstance = hPrevInstance;
   lpCmdLine = lpCmdLine;


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


Top
  
 
 Post subject:
PostPosted: Sat Nov 06, 2004 3:21 am 
kreely wrote:
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.


Top
  
 
 Post subject:
PostPosted: Fri Nov 19, 2004 3:13 pm 
Oh, how I wish the apps I inherited from my predecessor weren't giving me so much fodder to post in this thread.
Code:
CDDrawClass::~CDDrawClass() {
  if (m_pDirectDraw) {
    while(m_pDirectDraw->Release());
  }
}


Top
  
 
 Post subject:
PostPosted: Fri Nov 19, 2004 4:37 pm 
Pi wrote:
Oh, how I wish the apps I inherited from my predecessor weren't giving me so much fodder to post in this thread.
Code:
CDDrawClass::~CDDrawClass() {
  if (m_pDirectDraw) {
    while(m_pDirectDraw->Release());
  }
}


Wha...?

What would that even do?


Top
  
 
 Post subject:
PostPosted: Fri Nov 19, 2004 4:42 pm 
Offline
Energizer Bunny
User avatar

Joined: Wed May 22, 2002 12:24 am
Posts: 1634
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


Top
 Profile  
 
 Post subject:
PostPosted: Fri Nov 19, 2004 6:15 pm 
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.


Top
  
 
 Post subject:
PostPosted: Fri Nov 19, 2004 6:49 pm 
Pi wrote:
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.


Top
  
 
 Post subject:
PostPosted: Tue Nov 30, 2004 12:50 pm 
Offline
Nightstar Graveyard Daemon
User avatar

Joined: Mon Jun 03, 2002 8:30 pm
Posts: 1071
Location: Wouldn't you rather observe my Velocity?
We instantiate 4 objects of this class. Each object must be threadsafe.

Code:
// original code:
class CMediatorCallback {
   // ...
   CMutex m_ThingieMutex;
};

CMediatorCallback::DoCallbackThingie(void) {
    m_ThingieMutex.Lock();
    // ...
    m_ThingieMutex.Unlock();
}


Code:
// post cow-orker code
class CMediatorCallback {
    // ...
    // CMutex m_ThingieMutex;
};

CMediatorCallback::DoCallbackThingie(void) {
    static CMutex ThingieMutex;
    ThingieMutex.Lock();
    // ...
    ThingieMutex.Unlock();
}


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.


Top
 Profile  
 
 Post subject:
PostPosted: Wed Jan 05, 2005 1:53 pm 
Offline
Nightstar Graveyard Daemon
User avatar

Joined: Mon Jun 03, 2002 8:30 pm
Posts: 1071
Location: Wouldn't you rather observe my Velocity?
Code:
// In the header file:
std::vector<ToolIdleTimeInfo*> m_vToolIdleTime;

// In the cpp:
   ToolIdleTimeInfo *toolState = new ToolIdleTimeInfo;
   FILE* file = NULL;

   if( (file = fopen((LPCSTR)tchFilePath, "r")) != NULL )
   {
      while(!feof(file))
      {
         if(::fread(toolState, sizeof(ToolIdleTimeInfo), 1, file))
            m_vToolIdleTime.push_back(toolState);
      }
      fclose(file);
      m_vToolIdleTime.begin();
   }


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.


Top
 Profile  
 
 Post subject:
PostPosted: Wed Jan 05, 2005 3:54 pm 
Chalain wrote:
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.

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?


Top
  
 
 Post subject:
PostPosted: Tue Jan 11, 2005 9:57 am 
Offline
Nightstar Graveyard Daemon
User avatar

Joined: Mon Jun 03, 2002 8:30 pm
Posts: 1071
Location: Wouldn't you rather observe my Velocity?
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:

Code:
typedef enum { FOO_NONE, FOO_A, FOO_B } FOO;
typedef enum { BAR_NONE, BAR_A, BAR_B } BAR;
typedef enum { BAZ_NONE, BAZ_A, BAZ_B } BAZ;
typedef enum { QAZ_NONE, QAZ_A, QAZ_B } QAZ;
typedef enum { QUX_NONE, QUX_A, QUX_B } QUX;

...

FOO foo; BAR bar; BAZ baz; QAZ qaz; QUX qux;

foor = bar = baz = qaz = qux = 0;


(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:

Code:
foo = bar = baz = qaz = qux = QUX_NONE;


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.

Code:
#include <iostream>

using namespace std;

typedef enum { FOO_NONE, FOO_A, FOO_B } FOO;
typedef enum { BAR_NONE, BAR_A, BAR_B } BAR;
typedef enum { BAZ_NONE, BAZ_A, BAZ_B } BAZ;
typedef enum { QAZ_NONE, QAZ_A, QAZ_B } QAZ;
typedef enum { QUX_NONE, QUX_A, QUX_B } QUX;

int main(int argc, char *argv[]) {
    FOO foo; BAR bar; BAZ baz; QAZ qaz; QUX qux;

    // BWAHAHAHAAAA!
    foo = (FOO)bar = (BAR)baz = (BAZ)qaz = (QAZ)qux = QUX_NONE;

    cout << "FOO is " << foo << endl;
    cout << "BAR is " << bar << endl;
    cout << "BAZ is " << baz << endl;
    cout << "QAZ is " << qaz << endl;
    cout << "QUX is " << qux << endl;
}


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 115 posts ]  Go to page 1, 2, 3, 4  Next

All times are UTC - 6 hours [ DST ]


Who is online

Users browsing this forum: No registered users and 1 guest


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum

Search for:
Jump to:  
Powered by phpBB® Forum Software © phpBB Group