What Are the Hallmarks of Good / Bad Code?

[GregGreen]

I've worked with a couple of people like that. This is not an easy question to answer. Good code does what it is supposed to do, that is still the primary criteria. But good code also is minimal, cohesive, modular, clear, efficient and comprehensible. It's sort of like that old definition of obscene: you'll know it when you see it.

I'd add that good code is easily testable as well.

Nothing sucks more than going to update a dependency or add a new feature and not being able to easily confirm you didn't break everything.

 

[slugg]

If it reads kinda/sorta like small English sentences, then I like it. Sometimes, a paragraph is necessary. But generally speaking, I like code to be broken up into small methods, where each method does ONE and only ONE thing. They all work together, obviously, but then you end up with something that reads very naturally.

Getting code to work is easy. Keeping it working for 10 years is the hard part.

 

[IHateMyJob2004]

Good:

1) It works. Meaning there are requirements and the software implements them
2) It is unit tested
3) It is maintainable

Bad:
1) Doesn't work
2) No automated tests
3) Not maintainable

I think this is pretty much it. I work mostly in Java. Good unit tests and constant refactoring are amazing tools if you use them.

 

[Spungo]

If it reads kinda/sorta like small English sentences, then I like it. Sometimes, a paragraph is necessary. But generally speaking, I like code to be broken up into small methods, where each method does ONE and only ONE thing. They all work together, obviously, but then you end up with something that reads very naturally.

I like making code that reads a lot like Gerry's concept of Plain English programming. Get the basic concept down then define the functions later. Use functions that don't exist yet. Suppose you're programming blackjack. Start with something like this:

// start with 2 cards
DrawCard();
DrawCard();
// use GetSum() to calculate the sum

// draw cards until it's 17 or larger
while (GetSum() < 17)
{
   DrawCard();
}

Stay();

None of these functions exist yet, but you can look at the big picture and see if the general flow of it makes sense. Everything is nice and close together. It's easy to change something or move the pieces around.

 

[IronWing]

Comment everything. # Conveys thoughts on commenting code to forum.

For user interactive codes, store all on-screen text in variables, preferably all in one language file for ready conversion to other languages. This takes time but makes regionalization easier and makes it easier to clarify text when your users tell you they don't understand what the input is supposed to be.

Think about scaling. If you know your code will only be used on small data, optimization may be a waste of time but if there is a case where your code will be used at a larger scale think about the resources it calls. I wrote an addon script for an open source forum project that simply wished members a happy birthday on their birthdays on the forum front page. It worked great on my test forum where I had a good three visits a per day. Then I got the email from the guy with 2,000 members pointing out that my script opened every member's member file to check for birthdays (flat file model database) every time the front page loaded. Oops. He provided a five line fix that cut 2,000 file opens to one.

 

[Ken g6]

I'm going to have to amend that to "Comment everything intelligently." Or "Comment everything descriptively."

Bad: a=x // Assign x to a.

Good (possibly, depending on context): a=x // Store the x-position in the accumulator.

 

[Markbnj]

I'm going to have to amend that to "Comment everything intelligently." Or "Comment everything descriptively."

Bad: a=x // Assign x to a.

Good (possibly, depending on context): a=x // Store the x-position in the accumulator.

My attitude toward comments has changed over the years, possibly because I've been using ever higher-level languages. Comments are a trade-off. They may explain something that would otherwise be cryptic, but they unavoidably disrupt the flow and readability of the code itself. It's sort of like if you put all the footnotes in a history book into the text itself. Which is an interesting comparison, actually. In any case, I tend to think that almost all your code should be self-explanatory. Comments should be reserved for things that truly need explaining, and not added in the hope that it will save a programmer 90 seconds of work to read and comprehend something, because that will have to happen anyway.

 

[DaveSimmons]

I agree. I try to comment the unexpected and unclear not the normal actions of the code.

Using good variable names and function names should make most code self-explanatory.

If code is correct but "looks wrong" for some reason then I explain why it's meant to be this way. Or if possible I'll re-write without the oddness.

For a longer function or procedure I might add single-line comments to mark the beginning of new steps ("acquire resource handles", "build list of widgets", "fold, spindle and mutilate").

 

[Spungo]

Bad: a=x // Assign x to a.

I sometimes write like this if I'm keeping a running commentary in the comments. I do that because then I can read the code by looking at the comments alone. Imagine looking at the right side of the screen and seeing this:

// Draw two cards.

// If both cards are worth 10,

// split the hand.

// If the cards add up to 11, 

// double down.

// If the sum is less than 17,

// draw another card.

The comments will be reduced as I get better at this. At this time, I barely understand half of my code. Bits and pieces are taken from google searches.

 

[purbeast0]

My attitude toward comments has changed over the years, possibly because I've been using ever higher-level languages. Comments are a trade-off. They may explain something that would otherwise be cryptic, but they unavoidably disrupt the flow and readability of the code itself. It's sort of like if you put all the footnotes in a history book into the text itself. Which is an interesting comparison, actually. In any case, I tend to think that almost all your code should be self-explanatory. Comments should be reserved for things that truly need explaining, and not added in the hope that it will save a programmer 90 seconds of work to read and comprehend something, because that will have to happen anyway.

i agree with this.

my comments typically tend to explain the business logic behind the stuff and the reason for why i'm doing things, rather than what it is actually doing. my code is typically readable enough that it's obvious what it is actually doing.

variables should be named in plain english and read like plain english.

it's sad that in early programming classes (at least the ones i took in school) they put such an emphasis on comments and made you write so many stupid comments, and even block comments when it wasn't necessary.

 

[purbeast0]

I sometimes write like this if I'm keeping a running commentary in the comments. I do that because then I can read the code by looking at the comments alone. Imagine looking at the right side of the screen and seeing this:

// Draw two cards.

// If both cards are worth 10,

// split the hand.

// If the cards add up to 11, 

// double down.

// If the sum is less than 17,

// draw another card.

The comments will be reduced as I get better at this. At this time, I barely understand half of my code. Bits and pieces are taken from google searches.

this could easily just be written in code though, no need to look at comments to the right of all your code.

drawTwoCards();

if (sumOfFirstTwoCardsEquals(10)) {
  splitTheHand();
}
if (sumOfCardsEquals(11)) {
  doubleDown();
}
if (sumOfCardsLessThan(17) {
  drawAnotherCard();
}

comments simply aren't necessary if you have code written that way.

 

[dave_the_nerd]

Flawed assumptions and bad error handling.

I found a bit in one of our scripts today. There's an optional CLI parameter which SHOULD be a boolean, according to the documentation.

If the parameter isn't there, it's assumed to be false.

If the parameter IS there, then it takes the argument as a string, and if it has a value, it assumes that the value is true and goes from there.

if variable:
blah blah blah

So "VARIABLE=true" and "VARIABLE=false" have the same interpretation, but "VARIABLE=" is false. As is omitting the parameter entirely.

But I'm going to script around it since I don't want to do the paperwork to get a code change in the other repo.

 

[Spungo]

this could easily just be written in code though, no need to look at comments to the right of all your code.

I think it's hard to read code because it looks like it's written by a 5 year old. i R teh 1337 h4x0rz LoL

if (sumOfFirstTwoCardsEquals(10)) {
  splitTheHand();

Sometimes it looks easier to just put the comments at the top so they're all together.
// Draw two cards. Keep hitting until 17 or greater
DrawCard();
DrawCard();
if (CardSum() < 17) { DrawCard(); }
Stay();

 

[purbeast0]

I think it's hard to read code because it looks like it's written by a 5 year old. i R teh 1337 h4x0rz LoL

if (sumOfFirstTwoCardsEquals(10)) {
  splitTheHand();

Sometimes it looks easier to just put the comments at the top so they're all together.
// Draw two cards. Keep hitting until 17 or greater
DrawCard();
DrawCard();
if (CardSum() < 17) { DrawCard(); }
Stay();

what you put there isn't what you had the comments for above. the logic you have in the post i quoted here isn't the same as the logic you had comments for up above.

i also agree with you about putting the business logic up at the top of the block.

and yeah, function names can be dumbed down a bit, i simply gave that as an example because it reads like english. not sure how it reads like a 5 year old leet haxor. you must hate java.

 

[Ken g6]

Flawed assumptions and bad error handling.

This reminds me of one of the first jobs I ever had. I had to fix someone else's Java code, which I think was involved in backend stuff. Problem was it never reported errors because the original coder had added blanket try-catch statements that did nothing - not even any kind of error logging.

I didn't know it at the time - and the thing hadn't even been named yet - but this was my first introduction to a programming anti-pattern, specifically the Diaper Pattern.

it's sad that in early programming classes (at least the ones i took in school) they put such an emphasis on comments and made you write so many stupid comments, and even block comments when it wasn't necessary.

i also agree with you about putting the business logic up at the top of the block.

Sometimes block comments - at the top of a Class or Method - are necessary, though. Especially when writing a library when you want auto-documentation of the API to work. Like Javadoc, for instance.

 

[purbeast0]

This reminds me of one of the first jobs I ever had. I had to fix someone else's Java code, which I think was involved in backend stuff. Problem was it never reported errors because the original coder had added blanket try-catch statements that did nothing - not even any kind of error logging.

I didn't know it at the time - and the thing hadn't even been named yet - but this was my first introduction to a programming anti-pattern, specifically the Diaper Pattern.



Sometimes block comments - at the top of a Class or Method - are necessary, though. Especially when writing a library when you want auto-documentation of the API to work. Like Javadoc, for instance.

yeah i know, and i've done those plenty myself. but in your 1st or 2nd programming class in college, writing api documentation shouldn't be anything you are really worried about heh. you still have a very long way to go before writing api documentation, let alone an api.

but those aren't even the type of block comments i'm referring to, i'm just talking about in the middle of the file explaining stuff. i just remember so much emphasis on commenting while in college.

 

[Maximilian]

Does what its supposed to...

Good code :thumbsup:

 

[Cogman]

I think it's hard to read code because it looks like it's written by a 5 year old. i R teh 1337 h4x0rz LoL

if (sumOfFirstTwoCardsEquals(10)) {
  splitTheHand();

Sometimes it looks easier to just put the comments at the top so they're all together.
// Draw two cards. Keep hitting until 17 or greater
DrawCard();
DrawCard();
if (CardSum() < 17) { DrawCard(); }
Stay();

I disagree. Purebeasts's code is pretty legible and doesn't at all read like "I r l33t haxor"

Given that just a few months ago you started programming, it seems a little pretentious of you to start criticizing someone else's code like that.

I and most of my coworkers prefer code where comments are only used for API documentation and to help explain potentially confusing code. Commenting every single line is overkill and often it only serves to muddle up what is actually being done.

 

[IHateMyJob2004]

Regarding the card game example.

If you have properly named classes and methods the code is self documenting.

The only time verbosity is really needed is with engineering notes that explain why code was written a certain way. Documenting the decision making process for complex issues can be useful.... especially when documentation procedures suck or don't exist .

 

[slugg]

I like making code that reads a lot like Gerry's concept of Plain English programming. Get the basic concept down then define the functions later. Use functions that don't exist yet. Suppose you're programming blackjack. Start with something like this:

// start with 2 cards
DrawCard();
DrawCard();
// use GetSum() to calculate the sum

// draw cards until it's 17 or larger
while (GetSum() < 17)
{
   DrawCard();
}

Stay();

None of these functions exist yet, but you can look at the big picture and see if the general flow of it makes sense. Everything is nice and close together. It's easy to change something or move the pieces around.

That's exactly how I like code to look like. Now the *way* you program it is up to you. What you described is sometimes called top-down or consume-first development. With modern IDEs, they're smart enough to stub out each method as you go, which is pretty cool.

Most of the time, my way of doing things involves blasting away code in a single method. As soon as I hit a second abstraction level, I use the IDE refactoring tools to extract a method. For example, if I were coding the blackjack game in your example, I would have typed out all the code to draw a card, then I would have hit the while loop. At this point, we're now facing a problem that goes against my coding philosophy: our method is now doing more than one thing (draws two cards, then applies gaming logic). So then what I do is highlight all the "draw a card" code, then use the IDE's refactoring tools to extract a method (name it DrawCard, in this case). The method I started with now only does one thing (applies the game's logic) and it stays clean.

I also use the consume-first pattern; it just depends on what I'm doing. They're both valid options. One case where I'd likely use consume-first is when I'm writing something that's a bit complicated with a few unknowns. For example, let's just pretend that summing the values of a hand of cards was difficult, in my head, I would say "I know this is possible, but I'll figure it out later" and just type out "GetSum()". Let the IDE yell at me because the method doesn't exist, then let the IDE create it. If you set up your keyboard commands and code snippets/templates to facilitate this kind of development, you can seriously code SUPER fast.

When coding like this, I have found almost zero need for comments. I only really comment public API, and that's about it. If you're ever coding a weird algorithm that needs to break these rules, then sure, comment away. But seriously, 95% of the time, you don't need comments if your methods are small, do ONE thing, and read English-like.

Following this philosophy makes unit testing a breeze, by the way. Think about it; if each method does only one thing, how hard is it to write a single test for a single method with 100% coverage? It's so easy that it's even a bit formulaic, and you know what that means: code generation. You can pretty much auto-generate 75% of your unit tests using templates and a mocking framework.

So yea, go ahead and have 20 methods, even if you can do it in one. Optimize code for human consumption; compilers are way smarter than us mere mortals. Don't worry about making code "tight." That's my take, at least. I'd be happy to get some criticism on this.

 

[Spungo]

not sure how it reads like a 5 year old leet haxor. you must hate java.

VB is definitely the worst. Even the simple "goto" is written as GoTo.
ElseIf
UShort.

tRy ReAdInG sEnTeNcEs LiKe ThIs

IT'S ALSO DIFFICULT TO READ THINGS WRITTEN IN ALL CAPS

Things that are too long can get confusing as well.
SomeThing.Method().AnotherMethod(5, Object1.Method2(7)).OtherMethod().ToString();

It seems more readable when it's on multiple lines:

SomeThing
             .Method()
             .AnotherMethod(5, Object1.Method2(7))
             .OtherMethod()
             .ToString();

Actually that still looks terrible. The spacing can be tweaked until it's easier to follow what's going on.

 

[Cogman]

So yea, go ahead and have 20 methods, even if you can do it in one. Optimize code for human consumption; compilers are way smarter than us mere mortals. Don't worry about making code "tight." That's my take, at least. I'd be happy to get some criticism on this.

Interesting side note.

In Java, JavaScript, and the .Net languages (and probably future vmed languages) it is much better to write small methods than it is to write large methods. a LOT better. The reason is because most of those compilers progressively optimize and deoptimize code usually using the method as the border for where optimizations begin and end.

The problem with large methods is two fold. One, you increase the likelihood that you are going to do something which forces the VM to deoptimize, this is particularly true of javascript, and two you make it so that the VM spends more time on optimizing more code. So where the VM might only optimize an inner portion of code that is consuming 99% of the runtime, it instead has to optimize the whole shebang. That takes time.

So not only is it a good coding practice to have smaller focused methods, it is also a performance benefit.

For the likes of C and C++, this is less true, but generally the compiler is really good about choosing when to inline and when not to inline stuff. To get the best performance you pretty much have to do a profile guided optimization.

 

[dank69]

WTF? Never split 10s!

 

[Ken g6]

@dank69: Wrong thread?

 

[Cogman]

WTF? Never split 10s!

What are you talking about?