DIYEFI.org Forum

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

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.

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.

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.

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.

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

something new to ponder:
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 ?

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.

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.

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

Complete code coverage etc etc