View unanswered posts | View active topics It is currently Fri Mar 24, 2017 6:49 pm



Reply to topic  [ 30 posts ]  Go to page 1, 2, 3  Next
examples of bad code (and why) ? 
Author Message
LQFP112 - Up with the play
User avatar

Joined: Thu Sep 10, 2009 9:23 am
Posts: 244
Location: Dayton, OH
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:
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 ..


Wed Apr 14, 2010 6:14 pm
Profile
LQFP112 - Up with the play
User avatar

Joined: Thu Sep 10, 2009 9:23 am
Posts: 244
Location: Dayton, OH
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.


Wed Apr 14, 2010 6:24 pm
Profile
Moderator
User avatar

Joined: Tue Jan 15, 2008 2:31 pm
Posts: 14498
Location: Home sweet home!
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!


Fri Apr 16, 2010 9:18 am
Profile WWW
Moderator
User avatar

Joined: Tue Jan 15, 2008 2:31 pm
Posts: 14498
Location: Home sweet home!
Some double posting is about to occur:

Quote:
<constant name="twoStroke">&quot;Four-stroke&quot;</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:
 int RowsAffected = 0;
                RowsAffected = Database.ExecuteNonQuery(dbCommand);
                if (RowsAffected > 0)
                    return "Success";
                else
                    return "failed";
            }


Another with Upper case F.....

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


Fri Apr 16, 2010 12:25 pm
Profile WWW
LQFP112 - Up with the play
User avatar

Joined: Thu Sep 10, 2009 9:23 am
Posts: 244
Location: Dayton, OH
Quote:
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.

Quote:
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.



Fri Apr 16, 2010 4:33 pm
Profile
LQFP112 - Up with the play
User avatar

Joined: Thu Sep 10, 2009 9:23 am
Posts: 244
Location: Dayton, OH
here's typical 'boilerplate' of mine from a typical header .. what's wrong with it ? do you need more context ?

Code:

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 );



Fri Apr 16, 2010 4:41 pm
Profile
LQFP112 - Up with the play
User avatar

Joined: Thu Sep 10, 2009 9:23 am
Posts: 244
Location: Dayton, OH
something new to ponder:

Code:
/**
 * @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 ?


Fri Apr 16, 2010 8:27 pm
Profile
Moderator
User avatar

Joined: Tue Jan 15, 2008 2:31 pm
Posts: 14498
Location: Home sweet home!
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!


Sat Apr 17, 2010 1:48 am
Profile WWW
LQFP112 - Up with the play
User avatar

Joined: Thu Sep 10, 2009 9:23 am
Posts: 244
Location: Dayton, OH
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.


Sat Apr 17, 2010 3:29 am
Profile
LQFP144 - On Top Of The Game
User avatar

Joined: Mon Mar 31, 2008 12:47 am
Posts: 568
Location: New Zealand, land of the long white burnout
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


Sat Apr 17, 2010 9:11 pm
Profile WWW
Display posts from previous:  Sort by  
Reply to topic   [ 30 posts ]  Go to page 1, 2, 3  Next

Who is online

Users browsing this forum: No registered users and 2 guests


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
You cannot post attachments in this forum

Search for:
Jump to:  
Powered by phpBB® Forum Software © phpBB Group
Designed by ST Software for PTF. ColorizeIt.