examples of bad code (and why) ?

A place to discuss software and code that isn't automotive related. Free and open source preferred but not compulsory.
User avatar
EssEss
LQFP112 - Up with the play
Posts: 244
Joined: Thu Sep 10, 2009 9:23 am
Location: Dayton, OH

examples of bad code (and why) ?

Post by EssEss »

I was talking to Fred about bad code last night .. I've seen some .. but it sounds like I haven't seen code as bad as he has.

1) does anyone have links to some code out there which you consider bad ? C focused please.
2) can you explain what makes them bad in your opinion ? specifics.

I'm looking at this as a learning experience. I didn't know I was writing bad code until someone pointed out why .. maybe someone else can learn 'why' from some examples posted in this thread. I'm not looking for good code at the moment, I may start another 'good' thread later.

My motivation ? I'm told by the 'experts' to seek out examples of good code to read and learn from .. I may have been picking out the wrong things if I don't know that it's bad. By quickly browsing a codebase and getting the gut feel of 'this is good' .. I'll then dedicate some deeper time to see what I can learn from it. Kernel sources and Python codebase come to mind - but I'm always on the lookout for something different.

I don't want to get drowned out in the minutiae of spec's (if someone is really interested they can dig into it -- and maybe debunk it!). Just some quick one-liners/guidelines/rules-of-thumb of your opinion why its subpar. I'll start w/something from recent memory that I've seen in my own codebase:

Code: Select all

float f = 1.2;
if( f == 1.2 ) {
  doit();
}
a couple things stand out:
1) no 'f' suffix on the declaration - the compiler will most likely treat it as a double which may penalize you in performance later through silent casting
2) direct comparison to a float (well actually a double in this case since there is no 'f' suffix) - look to using <=, <, >, >= instead.

Architecture flaws also work here, but are harder to explain short-n-sweet. I'm off to look for some real world examples ..
User avatar
EssEss
LQFP112 - Up with the play
Posts: 244
Joined: Thu Sep 10, 2009 9:23 am
Location: Dayton, OH

Re: examples of bad code (and why) ?

Post by EssEss »

Even simpler .. just post a link and I'll figure out the rest if you don't have the desire or bandwidth to explain. I can scrub it and post up my own opinions on it.
User avatar
Fred
Moderator
Posts: 15431
Joined: Tue Jan 15, 2008 2:31 pm
Location: Home sweet home!
Contact:

Re: examples of bad code (and why) ?

Post by Fred »

1.2 is also a magic number! It should really have some sort of identifier. I'm going to get to this thread and post like crazy. I may link a few of my friends to it too and see if they can come up with any beauties!

Fred.
DIYEFI.org - where Open Source means Open Source, and Free means Freedom
FreeEMS.org - the open source engine management system
FreeEMS dev diary and its comments thread and my turbo truck!
n00bs, do NOT PM or email tech questions! Use the forum!
The ever growing list of FreeEMS success stories!
User avatar
Fred
Moderator
Posts: 15431
Joined: Tue Jan 15, 2008 2:31 pm
Location: Home sweet home!
Contact:

Re: examples of bad code (and why) ?

Post by Fred »

Some double posting is about to occur:
<constant name="twoStroke">"Four-stroke"</constant>

NOT Otto OR YES?

how about :

<constant name="Strokes">Four</constant>

What do you think?

I think that if they are going to do it that way, they should do it like this :

<constant name="twoStroke?">Four-stroke!</constant>
Fred wrote:OMFG - Please see post subject!

Especially not to : Bangalore, India.

interface has function that returns a string....

Code: Select all

 int RowsAffected = 0;
                RowsAffected = Database.ExecuteNonQuery(dbCommand);
                if (RowsAffected > 0)
                    return "Success";
                else
                    return "failed";
            }
Another with Upper case F.....

Code: Select all

                    msg = _controller.UpdateCostCentre(update);
                    if (msg == "Success")
                        return "Cost Centre record has been Successfully Updated";
                    else
                        return "  Failed to Update Cost Centre or Cost Centre already exist";
And the good Polish developer said - "They don't know boolean type??"

epic fail!

-----

Project Manager to Indian developers :

"Do you even know what a stack trace is?" - LOL

"Did you take a tag? Do you know what a tag is?

... they didn't tag anything for release...

Hell, even I tag the FreeEMS releases manually! Shocking!

----

CamelcasedoneWrong

Note, lower case in the middle and upper on the front.

-----

Unreachable code :

throw exception
return xyz

-----

There was an if 0 do xyz, if > 1 do abc, what the hell happens if it was exactly 1 ???

-----------

email files around instead of SVN etc

-------------

variable1=variable2 = functionxyz();

^ LOLOLOL

(inconsistent spacing intentional...)

-------------

And one of the more potentially expensive ones :

Code: Select all

((exchangeRate == 0) ? 1 : exchangeRate)
Oh, there is no such country as fredWorld, just give them one to one exchange of currency with the great british pound LOLLOLOLLLOFOFLOLOL

--------------

This stuff leaves the MegaSquirt C for dead! I can't believe how bad this is, it's not just "me being fussy" this is genuinely awful!!

Fred. - ROFLing around the office in fits of laughter and disbelief!
Fred wrote:

Code: Select all

if (session.getRecord().getId() == 11) { //do something }
^ more LOL - hard coding the id of a database record... OMFG... these guys are REALLY BAD!!
Fred wrote:Also from the same code base :

171 line long for loop!!

:-)
Fred wrote:Interlude :

Code: Select all

if(blnTrueOrFalse == true){
      otherBooleanVariable = false;
}
if(blnTrueOrFalse == false){
      otherBooleanVariable = true;
}
That is ten types of wrong!

use else
just check the boolean, not compare it with true/false
just assign it straight to the output var with or without notting
use a name that means positive for positive, not the equiv of maybe

jesus, these guys are ROUGH...
Except for the first quote from MegaSquirt XML config. the above was all Indian C# code. Outsourcing is evil and stupid. Bad quality costs more than good quality in the short, medium and long term.

Fred.
DIYEFI.org - where Open Source means Open Source, and Free means Freedom
FreeEMS.org - the open source engine management system
FreeEMS dev diary and its comments thread and my turbo truck!
n00bs, do NOT PM or email tech questions! Use the forum!
The ever growing list of FreeEMS success stories!
User avatar
EssEss
LQFP112 - Up with the play
Posts: 244
Joined: Thu Sep 10, 2009 9:23 am
Location: Dayton, OH

Re: examples of bad code (and why) ?

Post by EssEss »

Unreachable code :

throw exception
return xyz
.. and the warning was probably ignored
my rule: once branched - there better not be any spew !

when someone asks me why their code works sporadically, the first thing I check is compiler spew.

that is, if you're lucky enough to work for a place that uses source control. diff is powerful and understood instantly the first time it saves your ass in 3 minutes.
magic number
good call - blew right past me.
Last edited by EssEss on Fri Apr 16, 2010 4:46 pm, edited 1 time in total.
User avatar
EssEss
LQFP112 - Up with the play
Posts: 244
Joined: Thu Sep 10, 2009 9:23 am
Location: Dayton, OH

Re: examples of bad code (and why) ?

Post by EssEss »

here's typical 'boilerplate' of mine from a typical header .. what's wrong with it ? do you need more context ?

Code: Select all


typedef enum encodeErr_t {

  ENC_OK = 0,
  ENC_DST_FULL = 1,
  ENC_BAD_DSTPTR = 2,
  ENC_BAD_DSTLEN = 3,
  ENC_UNKNOWN = 4

} encodeErr_t;

encodeErr_t  someFunc( args );

User avatar
EssEss
LQFP112 - Up with the play
Posts: 244
Joined: Thu Sep 10, 2009 9:23 am
Location: Dayton, OH

Re: examples of bad code (and why) ?

Post by EssEss »

something new to ponder:

Code: Select all

/**
 * @public
 * @brief turn on load indicator
 */
static inline void loadLEDOn( void ) {

}
I name funcs like this: [things]+[action]() -> [loadLED]+[On](), but I notice the conflict with how I 'naturally describe it' in the comment.

As I just typed this out moments ago (literally), I remember that Fred pointed this out to me before, but I dont know which way he leans .. or what most people prefer.

btw: this is in a header, hence the @public, even though I marked it static

opinions ?
User avatar
Fred
Moderator
Posts: 15431
Joined: Tue Jan 15, 2008 2:31 pm
Location: Home sweet home!
Contact:

Re: examples of bad code (and why) ?

Post by Fred »

EssEss wrote:if you're lucky enough to work for a place that uses source control.
What??? How is this lucky? If there is source code, there should, at the least be svn. If an employer wasn't going to allow a solid version control system, either I would use my own locally, or resign!

You had better explain your examples, too. I didn't see the issues with them (C noob).

Fred.
DIYEFI.org - where Open Source means Open Source, and Free means Freedom
FreeEMS.org - the open source engine management system
FreeEMS dev diary and its comments thread and my turbo truck!
n00bs, do NOT PM or email tech questions! Use the forum!
The ever growing list of FreeEMS success stories!
User avatar
EssEss
LQFP112 - Up with the play
Posts: 244
Joined: Thu Sep 10, 2009 9:23 am
Location: Dayton, OH

Re: examples of bad code (and why) ?

Post by EssEss »

Fred wrote:
EssEss wrote:if you're lucky enough to work for a place that uses source control.
What??? How is this lucky?
ya - i just setup my own locally .. in my experience it's pretty normal for an EE to come up with a 30pg list of why source control is 'not for them'.
Fred wrote:I didn't see the issues with them
I guess thats good, neither do I .. but I hope someone does. I'm just throwing random stuff out there in the hopes that someone keys on something that I may be oblivious to.
User avatar
sry_not4sale
LQFP144 - On Top Of The Game
Posts: 568
Joined: Mon Mar 31, 2008 12:47 am
Location: New Zealand, land of the long white burnout
Contact:

Re: examples of bad code (and why) ?

Post by sry_not4sale »

Apparently the sqLite codebase is one of the best examples of C out there...

Complete code coverage etc etc
Owner / Builder: 1983 Mazda Cosmo 12at (1200cc 2-rotor turbo) coupe [SPASTK]
165hp @ 6psi standard - fastest production car in japan Oct 82
Post Reply