The Old Joel on Software Forum: Part 4 (of 5)

Good vs Bad Code

Can you objectively differentiate good code from bad code? I’ve seen my fair share of what I would consider bad code. I’ve also seen a lot of good code. The bad code has made me question the competence of the original author and curse his/her name regularly as I tried to make it work or add new features. The good code on the other hand has been a joy to work with (and often a learning experience) and has made making changes or adding new features a breeze.

Here is my list of the top 5 signs of bad code. If you would like, please add yours to the list.

mixing presentation, business logic, and database access layers
repeated code
obnoxiously long methods
little or no documentation and cryptic method names
god classes, classes with too many methods/properties, and insane object hierarchies

GuyIncognito
Tuesday, February 10, 2004

Hello, I'm traits of bad code. I've been discussed a hundred zillion times. I'm dead.

Joe
Tuesday, February 10, 2004

Too many abstraction layers
Code that doesn't serve any practical purpose
Documentation that is out of sync with the code
Functionality divided across too many classes

No, there are not any exact metrics, only rules of thumb and experience.

In my experience, super smart people tend to write messy code and cope with it just fine, while less bright individuals like myself have to keep things simple just so we can comprehend our own stuff.

Big B
Tuesday, February 10, 2004

What are 'god' classes?

Tuesday, February 10, 2004

<In my experience, super smart people tend to write messy code ... >

My experience has been precisely the opposite.

Canuck
Tuesday, February 10, 2004

The 'god classes' are those that are omnipresent (called from everywhere in the app and/or calls everything in the app), omnipotent (does everything and anything), and omniscient (knows about all data).

NoName
Tuesday, February 10, 2004

See "god classes" at http://www.phppatterns.com/index.php/article/articleprint/31/-1/1/

GuyIncognito
Tuesday, February 10, 2004

Joe,

This is in response to...

(quote)
All code is ugly except to the original programmer who only understands what he wrote at the time it was written.
(/quote)

GuyIncognito
Tuesday, February 10, 2004

Guy,

You have a pretty good starting list. Add this one to your list "Ego centeric programming" or "Youth-obsessed programming".

I've also witnessed the label of "bad" code given to projects taken over by another developer. It's the proverbial bashing of the old programmer. It goes something like this:
"new" programmer takes over project from "old" programmer (since he/she has moved on to a new project/job). "New" programmer says that all the old code is crap and he can't possibly workaround it. He will have to rewrite everything from scratch. After 6 mos "new" programmer finds a new gig and "even newer" programmer takes over the project. "Even newer" programmer says that the "old new" code is hideous and he will have to rewrite everything from scratch and so on...

It's the stuff that all infamous re-writes are made from.

Smitty
Tuesday, February 10, 2004

Most/all of your list is part of the "Code Smells" list in Fowler's _Refactoring_ book. I'd strongly recommend reading it if you haven't, and not just for the refactoring stuff; it's got some good insights into good OO design.

Chris Tavares
Tuesday, February 10, 2004

Good code: any code I wrote
Bad gode: any code I didn't write

(Required)
Tuesday, February 10, 2004

My thread on a similar subject:

http://discuss.fogcreek.com/joelonsoftware/default.asp?cmd=show&ixPost=110778&ixReplies=37

The overwhelming badness I encountered in this project was due to excessive use of abstractions and abstract layers invented for their own sakes, as well 'God' classes all over the place. And something like 370 separate 'base library' source modules, all commingled with no real layering or structure.

Unfortunately, almost nobody in that thread wanted to believe that either the client or the project was irredeemable, instead hewing to the politically correct "man with the money is always right" wimpout argument.

Sometimes, something is just absolute shit and you are obliged to call the inventor on the fact and wash your hands of it.

Ever Suffering Dolt
Tuesday, February 10, 2004

Oh, one other thing. I'm really tired of pseudo-enlightenment such as sarcastically meant statements like "anything I do is wonderful, anything anyone else does is crap."

*Professionals* judge their own work and the work of others when necessary.

I've worked very very hard to build up my skills. I 'call' hacks who get in the way, and sometimes that means that I bite the hand that supposedly feeds me.

I believe that qualified professionals are obliged to *not* suffer fools.

Ever Suffering Dolt
Tuesday, February 10, 2004

Addendum:

Good code: any code Dolt wrote
Bad gode: any code Dolt didn't write

Smitty
Tuesday, February 10, 2004

Dolt,

Just kidding. I thought it would be funny.

Smitty
Tuesday, February 10, 2004

Good one. ;-)

Ever Suffering Dolt
Tuesday, February 10, 2004

I've written gobs of bad code at times, usually because of time constraints, bad mood or being too damn exhausted to think of a better way. This isn't just stupid stuff due to inexperience either. Sometimes I laugh at the crap I've written. We're not always at the top of our game.

In my own defense, I still think my bad code isn't as bad of some of the garbage out there *shudder*.

The bad programmers are the ones that can't see the bad code, or don't admit they've written bad code and refuse to change.

Tuesday, February 10, 2004

Over-engineered code. This is where you have 10 layers of abstraction when 2 would be fine, all classes implement an interface, indirection is used everywhere.

Should be working
Tuesday, February 10, 2004

You can make rules, so long as you understand that there are exceptions to every rule.

For example, sometimes it's just plain not worth the extra time to seperate database/business/presentation layers. On a long running large project it is, but if it's something you want to get running as quickly as posible, it may not be.

Sometimes it's quicker to cut and paste rather than refactor if what you're doing is just a quick hack, and not part of the main system (every dev house has a few in house utilities that don't have to be perfect, they just have to work).

And so on.

To me, you can only judge good and bad code in context. What's good code in one project might be bad code in another. Look how both too much and too little abstraction are listed in this thread! Yet it's possible that the authors of such comments are referring to the same style of code, just used in different contexts.

Perhaps it's best to say that good code is code that approaches the sweet spot on the maintainability/cost/performance/functionality scales (which varies depending on what task the code is being put to). Bad code is everything else. ;)

Sum Dum Gai
Tuesday, February 10, 2004

This has been re-hashed a million times -

Let's say it a different way:

There is always some dood with 20-20 hindsight that comes along and critiques your code.

Maybe there were valid reasons for all of the levels of indirection? You'd be complaining too if it wasn't flexible enough and why didn't they think of the future.

The optimum amount of indirection changes with time.

Oh, and, whats good code in a prototype/demo may be lousy code in a production system. It depends on context too. If someone has abused the context, you can expect "lousy" code.

pdq
Tuesday, February 10, 2004

In the pursuit of more unified field theories --- <g>

Throughout my career, I have rarely heard anyone complain about code that was "too small" or "not complex enough". Unless they were going for a political L.O.C. distinction in their company...

A common hallmark of "bad code" seems to simply be - too big, too much, excessively complex, too many layers of abstraction.

That seems to be the common thread in most of these bitch-fests. Just too friggin' much code to solve a given task.

Other sins seem to be more amenable to resolution. "Big" code confuses issues, keeps you from understanding the basic problem being addressed, messes with your mind.

Agreed, all? Or is even this up for grabs?

Bored Bystander
Tuesday, February 10, 2004

Agreed.

Do the simplest thing that could possibly work...

Guy Incognito
Tuesday, February 10, 2004

But no simpler! ;)

Sum Dum Gai
Tuesday, February 10, 2004

Guy, you started off your conversation with a point that caught my eye as a hallmark of how subjective "bad code" is.

"mixing presentation, business logic, and database access layers"

...but then you just said...

"Agreed.

Do the simplest thing that could possibly work..."

The simplest thing that could possibly work is to thoroughly enmesh presentation, business logic, and database access. Separating them _is_ work, and often it is unjustified work. I've seen countless McProgrammers that carefully built a large set of presentation, business and data components without the slightest clue of the practical purpose for said separation in some cases.

Dennis Forbes
Tuesday, February 10, 2004

I'll second (or third or fourth or fifth) "too many layers of abstraction". You know, the sort of code that JRR Tolkien would write if he were a programmer -- an elaborately constructed cosmology filled with oh-so-clever sidestories that have nothing to do with the plot. Extra points given if the programmer in question invented an entirely new (but unneeded) language constructed for the express purpose of writing the app.

I'll add a new one that hasn't been mentioned: copy-and-paste code.

Alyosha`
Tuesday, February 10, 2004

2 sitting ducks:

1. How much duplicate code is there?
2. How many bugs can you spot with a quick scan?

I took over a driver from someone a few years back. She'd been working on it for 6 months and it wasn't done. The boss gave up and asked me to look at it. This stuff had routines 1-2 pages long, duplicated with minor differences. For example, volumeUp() and volumeDown(). Page and a half each. 2 lines where different (+= vs -=, and if value > max vs if value < 0).

I rewrote that sucker in about a week. Worked like a champ, was easier to modify, no bugs, and much smaller.

Snotnows
Tuesday, February 10, 2004

"Can you objectively differentiate good code from bad code?"

Sure. All you have to do is objectively define good vs. bad.

Sam Livingston-Gray
Tuesday, February 10, 2004

I have not seen some horror code like others on this forum, but bad code annoys me less than well formatted code. Bad code just makes me feel better about myself.

m
Tuesday, February 10, 2004

I'm going to get into an arguement with pretty much everyone whose posted in this thread, because I disagree. I've seen time and time again programmers come into a project, complain about "utterly useless" layers of abstraction, complain that the code is "over complicated", "too big", etc. etc. Way too many people from the single tier perl or ASP world just don't understand that just cause they're simple little design worked for them, that it doesn't mean it'll work wtih anyone. I've seen plenty of people on this board talk about how they don't even abstract their data tier from their presentation tier! Of course, soon as they see some code with a Factory Pattern and some "good for nothing, overly complicated interfaces", they think its junk. Good code is just that..."good". It has nothing to do with complexity or simplicity.

vince
Tuesday, February 10, 2004

"Way too many people from the single tier perl or ASP world just don't understand that just cause they're simple little design worked for them, that it doesn't mean it'll work wtih anyone. I've seen plenty of people on this board talk about how they don't even abstract their data tier from their presentation tier!"

`Simple little designs' -- good condescending start. The humorous thing is that there is absolutely nothing complex about multi-tiered designs or high levels of abstraction, but there are plenty of simpletons who have been fooled into thinking that's the case.

"I've written an incredibly complex sentence - `the and the and the and the and the and'"

Here's the thing though, vince: I haven't seen anyone in here say that there isn't a place for high levels of abstraction, and a carefully tiered architecture, and I think you're confused if you think that you're standing on a pedestal of righteousness in a sea of infidels.

What people are complaining about is completely useless, masturbatory over-coding. There _is_ a place where the simplest little perl script is by far the best solution, though there are those who will assure you that such a need surely must merit long winded discussions about how to properly generalize the case, blah blah blah, all so the code can eventually live its days until the source control node finally has a "destroy permanently" applied to it.

Quick question, Vince -- What was the original need for a data tier? To abstract you from the database -- you see in the good old days we had all sorts of crazy databases, each with their own proprietary APIs and funky interfacing methods, so it quite simply made sense to generalize your database access into a plug-in architecture: A data tier. Soon the database vendors all agreed, at least on the Microsoft platform, to increasingly standardized interfacing methods, JDBC and OLEDB (abstracted further in ADO and ADO.NET). From the classical perspective ADO is the data tier, though it's amazing how hard it is to push this into a McProgrammers head: They saw an episode of the MSDN Show one time where a guy said that it made sense to separate into presentation, business and data tiers, carefully separating out into CRUD layers (which horribly ties the data implementation to the physical data model...but hey -- don't tell the monkeys). It is these senseless, mindless drones that I find it intolerable to talk to.

Dennis
Tuesday, February 10, 2004

God classes are what you find in outsourced code. It is where "object oriented design" consists of putting the entire application inside a single object.

BM
Tuesday, February 10, 2004

> Agreed, all? Or is even this up for grabs?

Merely "too complex" doesn't account for this little gem from http://discuss.fogcreek.com/joelonsoftware/default.asp?cmd=show&ixPost=111815 ...

If w1223434 Then
SqlDoSomething(sSql,SqlHandle)
Else (if w213213 and w423424) or (w321233213 and w23123213) Then
SqlDoSomethingElse(sSql,SqlHandle)

... i.e. badly named identifiers. As a maintenance programmer I think I'd prefer to chase a multi-layer system accross its COM interfaces than to try to decypher "w1223434".

"Say what you mean, and mean what you say" ...

As for examples of "bad code", in addition to the "smells" in the _Refactoring_ book there are many other examples of bad code in _Code Complete_, helpfully flagged as "Coding Horror".

Christopher Wells
Tuesday, February 10, 2004

Hit a nerve, did I dennis? :-)

My point was that just because something is complex, doesn't mean its bad. I didn't say it meant that it was *good* either. But it seemed as if people were immediatly classifying anything complex as "bad code".

"separating out into CRUD layers (which horribly ties the data implementation to the physical data model...but hey -- don't tell the monkeys)"

Not sure I understand... How does seperating out the CRUD logic in any way tie the data implementation to the physical model? The whole idea of having your data in a Business Object (or data object or entity or DataTable or whatever) is that even *if* the physical model changes, you don't have to change your Business Object.

vince
Tuesday, February 10, 2004

God, I get sooo bored with the narcissistic wankers on this board.

get over yourselves
Tuesday, February 10, 2004

Then why are you here? Seriously? If you have such an issue then please move along and leave us to our group preening.

.
Tuesday, February 10, 2004

> Bad code just makes me feel better about myself.

Brilliant! I agree totally. It can be so relaxing to be able to just go in and refactor crap - you get a great sense of accomplishment and sense of self worth and competance. This is what I love about maintenance programming -- fixing other people's disasters.

The secret is to ignore all the time pressure and just pace yourself.

Tony Chang
Tuesday, February 10, 2004

Can any one point to any sites or reccommend any books that
could help in introducing best practices for writing "Good Code"?

McProgrammer
Tuesday, February 10, 2004

http://www.mcdonalds.com/

Manager
Tuesday, February 10, 2004

"best practices" == "way the author does it".

I find it to be a bullshit bingo buzzword (sorry for the alliteration). It's just a way for managers to beat people over the head. We conform to best practices to synergise our market leading product with e-commerce oppourtunities!

The best way to learn good vs bad code is to use your head. Look at a piece of code. Think about alternatives. Ask yourself what makes sense. Stuff whether what makes sense is a "best practice", a number of best practices are remarkably stupid, or don't apply to your situation.

The way to write "good code" is to learn to think critically about code. The answer doesn't lie in any book or on any web page.

Sum Dum Gai
Tuesday, February 10, 2004

Good code = code that people are willing to pay for
Bad code = code that people are not willing to pay for

Norrick
Wednesday, February 11, 2004

That definition is too simplistic.

Imagine a product that has been hacked away on to get into a position to release it. The code is a horrid mess, but it works well enough to make a product people want.

By your measure, it's good code.

Now, 6 months later, your competitor releases their product, that does everything yours does, and then some.

Suddenly you've got a problem. Nobody wants to buy your product, and you're going to have to spend a year to catch up to your competitor, because it's so hard to add new features to your mess of a codebase.

Suddenly it's not looking quite so good, is it?

That people are willing to pay money for it may be a necessary quality of good code (debatable, since some people give good code away, eg Open Source), it's certainly not the only criteria.

Sum Dum Gai
Wednesday, February 11, 2004

I have never met a programmer who thought that their code was bad. I'm met plenty of programmers who thought that their code from times past was. If you look at it like that, the code your writing today is bad.

Jack.

Jack of all
Wednesday, February 11, 2004

"That definition is too simplistic.

Imagine a product that has been hacked away on to get into a position to release it. The code is a horrid mess, but it works well enough to make a product people want.

By your measure, it's good code.

Now, 6 months later, your competitor releases their product, that does everything yours does, and then some.

Suddenly you've got a problem. Nobody wants to buy your product, and you're going to have to spend a year to catch up to your competitor, because it's so hard to add new features to your mess of a codebase.

Suddenly it's not looking quite so good, is it?

That people are willing to pay money for it may be a necessary quality of good code (debatable, since some people give good code away, eg Open Source), it's certainly not the only criteria. "

Thank you for openig up that dialogue. You are correct; whether or not people will pay for code is not the only criteria. However, I threw it out there all by its lonesome because programmers all too often forget that in addition to extensibility, scalability and maintainability, the project has to be marketworthy, or there won't BE a project anymore.

But if I had said all that in the first place, nobody would have noticed the part about being able to sell the product. ;) Geeks need to up their biz I.Q. big-time.

Norrick
Wednesday, February 11, 2004

In my experience, the one thing which contributes to understandable (my definition of "good") code is tight control of variable lifetime. Code Complete devotes a section to this and it really clarified things for me.

One can measure things such as the number of lines over which a variable is in scope and the "distance" between variable declaration and assignment. In almost every case, there is a direct correlation between these numbers and maintenance costs.

It certainly works for me. There are certain areas of our system which I shy away from. Without exception, they're 800-line procedures with 30+ variables, 6 levels of conditional nesting and inline procs which rely on side-effects. And this is where I introduce my pet rant against people who treat the rdbms as a giant global variable store and just pass temporary values in and out at will. Oracle Forms is particularly egregious as every data block item and form parameter is accessible globally from any program unit or trigger. Forms actually has a facility to define global variables but nobody uses it because everything's global by default.

Getting back on topic, I think that well-chosen variable names only become important as their lifetime increases. In a 5-line function, each variable could be just a single letter and the code clarity would only slightly suffer, but take a 50+ line function with 10 or more variables and good naming starts to make a difference. I tend to associate variable name length with lifetime and it's more effort for me to parse code which doesn't fit this pattern.

Paul Sharples
Wednesday, February 11, 2004

I try very hard not have variable lifetimes anywhere near 50 lines. I don't write paragraphs of English that long, damned if I'm going to do it in code. This is for a number of reasons:

1. It's too long to see on a 25x80 terminal screen. And I write code on those when I'm working late at night (don't need my glasses to read it...)

2. I have a lousy memory. 50 lines long is too long for me to remember what I'm doing.

Actually, there aren't any others. Number 2 alone means I can't read methods 50 lines long reliably so I strongly doubt I can write them.

Katie Lucas
Wednesday, February 11, 2004

"Geeks need to up their biz I.Q. big-time."

Maybe. However, they're also vary rarely given sufficient information to be able to make those kinds of judgements, and even more rarely given the power to act on anything business related.

There are a hell of a lot of jobs where the techical staff are completely isolated from the users. And I don't mean just that they never meet any users first hand, they also don't get any second hand information - like who's using the product and how, what features the users want (vs what features marketting dreams up), etc.

Under such circumstances, can you really blame the geeks for not knowing what will and wont make money? Most of the time management doesn't know either, that's why so many tech firms go broke! ;)

Sum Dum Gai
Wednesday, February 11, 2004

<<In a 5-line function, each variable could be just a single letter and the code clarity would only slightly suffer, but take a 50+ line function with 10 or more variables and good naming starts to make a difference.>>

This is true, but bear in mind that the routine that begins as a five line function may grow as features are added and defects are fixed.

Ran
Wednesday, February 11, 2004

Common factors in bad code:

(i) lack of logical breakdown/presentation into ideas and/or functions

(ii) not enough blank lines and spacing, used to organize the ideas into "paragraphs"

I think it therefore is obvious better code could would be more logically presented and make *more* use of blank lines and spacing.

Thus by induction:

Perfect code - would break down ideas and functions completely - entirely into whitespace.

S.Tanna
Wednesday, February 11, 2004

Bad code:
void main()

Good code:
int main(void)

Mastercard moment...
Professors teaching void main...priceless...

Prickulator
Wednesday, February 11, 2004

Probable a lot of what I'll list has been mentioned elsewhere like Code Complete or above but from my experience...

1/ Obfuscated code. The heavy use of Macros mainly to reduce the amount of typing and not much else. Increadibly difficult to debug because you can't step into a macro.('C'/'C++')

2/ Functions/Macros with side effects. A function does something but may do something else like setting a global in an unexpected way. I'm sure you can think of other situations.

3/ Viral code. I'm not talking about virus writing. I'm talking about hard logic that should only live in one place but is instead scattered all over the source code. If one of the places need modification then all of them need modification. The painful thing is that not all locations are identical and may have to be treated uniquely. [Shudder]

4/ Non planned or stream of consciousness coding. Sometimes ok but usually not the greatest.

5/ My old code. :)

Zekaric
Wednesday, February 11, 2004