Everyone would agree that code that isn’t syntactically valid is bad, if it is even considered code at all. But is code that works but is slow bad? What about code that works but can’t be easily followed? What if you can’t follow it but others can? What about if you can follow it and other can’t? What about code that fails in one corner case but works for the purpose it is used for? What about code without tests or documentation?
Code that is slow can be good since if it can be easily replaced then it can be good enough for now. That code that failed one corner case can be just what you need, if its limitations are documented correctly. I’m not going to call these cases bad code, but it definitely isn’t good code.
But, if it can’t be followed by the other developers on your team then you’ve probably got bad code. If you drop in F#/Clojure in the middle of some C#/Java application then the code could be good in another context but may not be in this application. If you whip out some obscure language feature that you don’t socialize appropriately among the team you could quickly produce some bad code in the context of this project.
Code without tests or documentation gets much trickier to evaluate. You don’t necessarily need to have tests or documentation (by which I mean java doc style comments) to have good code, but they tend to go together. If you’re going to leave those pieces out, you do need to have simpler interfaces and types. For instance, take
public bool IsPrime(int number)
That would be a perfectly good interface,even without documentation. You’ve got one simple input, one simple output and a name that is both understandable and easily searchable. Whereas,
public void RevertOFRTTrans(HttpContext context, OFRT thing, Transaction trans, bool isNew)
This has a bunch of problems. First you’ve got a cryptic acronym – what’s an OFRT? Hopefully it makes sense in your domain, or maybe the OFRT type has some documentation. The arguments are their own set of problems. What does this have to do with a HttpContext? Is a null HttpContext valid? Is trans the transaction being reverted or some other transaction? And what does isNew mean? Is it a new OFRT or a new Transaction, or maybe something else?
Add to that the question of what kind of exceptions this can throw. The transaction seems like it could throw database related exceptions. Maybe also some network-related ones since it is using an HttpContext. Maybe there are custom exception types that might be relevant. Plus all of the normal runtime exceptions.
When I had previously thought about this subject I had done some searching and found this article by Scott Hanselman that discussed the idea framed by Maslow’s hierarchy of needs. That was more software, or system level than the code level.
If it isn’t valid in the language it isn’t really code, and it is clearly deficient. This can be especially troublesome in interpreted languages where you wouldn’t even know until runtime.
Does What It Looks Like
This encompases clarity of intent and behavior, like what I discussed above with the RevertOFRTTrans example. If you look at it and don’t know what it is supposed to do, then it probably isn’t going to be doing it. If it looks like it does one thing and does another you won’t be happy using it or modifying it. This would also cover correctness, which – like syntactically valid – is generally a fundamental requirement of good code. For instance isPrime(3) better be true.
Tested and Documented
This moves past just doing what it looks like to formally telling and showing you what it does. It’s also two views of the same idea. The documentation is to clarify for the person using the code, and the tests are to clarify for someone who needs to modify the code.
Now, the code is both internally consistent and matches broader framework or language conventions. You could achieve this level without testing and documentation in some regards, but in many languages, matching the language’s conventions would imply that you have the documentation. This can be little things like Id vs ID or color or colour in names, or big things like how classes and namespaces are organized.
Enjoyable to Work With
This represents a piece of code that flows. You can change and extend it with confidence. You write other code that references it easily and that code also works well. Most code never gets to this level. Part of the reason is that you don’t interact with most code on a daily basis, and part of the reason is that you don’t sit and deeply refactor something until it is just right. We’re often willing to stop at good enough.
There are lots of reasons to stop at good enough. The schedule doesn’t allow for more. You committed a version that worked and you never got back to it. It wasn’t a piece of code intended for long-term use. You didn’t have a strong grasp of the domain and sort of muddled through and you know it’s not right but don’t know what to with it. Each of these reasons can be the right thing to do in either a professional or hobby project but you can push yourself and your teammates to do better. We should strive for more because if you reach for the stars you might still make it to the moon.