Ayende @ Rahien

Refunds available at head office

On Professional Code

Trystan made a very interesting comment on my post about unprofessional code:

I think it's interesting that your definition of professional is not about SOLID code, infrastructure, or any other technical issues. Professional means that you, or the support staff, can easily see what the system is doing in production and why.

It is a pretty accurate statement, yes. More to the point, a professional system is one that can be supported in production easily. About the most unprofessional thing that you can say is: “I have no idea what is going on.”

Expanding on this, we have been paying a LOT of attention recently to production readiness. We can’t afford not to. Just building the software is often just not enough for us. In many cases, if there is a problem, we can’t just debug through the process. Either because reproducing the problem is too hard or because it happens at a client side with their own private data. Even more important than that, if we can give the ops team the tools to actually see what is going on within the system, we drastically reduce the number of support calls we have to take.

Not to mention that software that actively support and help the ops team gets into the actual data center a lot faster and easier than software that doesn’t. Sure, clean code is important, but production ready code is often not clean code. I read this a long time ago, and it stuck:

Back to that two page function. Yes, I know, it's just a simple function to display a window, but it has grown little hairs and stuff on it and nobody knows why. Well, I'll tell you why: those are bug fixes. One of them fixes that bug that Nancy had when she tried to install the thing on a computer that didn't have Internet Explorer. Another one fixes that bug that occurs in low memory conditions. Another one fixes that bug that occurred when the file is on a floppy disk and the user yanks out the disk in the middle. That LoadLibrary call is ugly but it makes the code work on old versions of Windows 95.

Each of these bugs took weeks of real-world usage before they were found. The programmer might have spent a couple of days reproducing the bug in the lab and fixing it. If it's like a lot of bugs, the fix might be one line of code, or it might even be a couple of characters, but a lot of work and time went into those two characters.

Some parts of the RavenDB code are ugly. HttpServer class, for example, goes on for over thousands lines of mostly error detection and recovery modes. But it works, and it allows us to inspect it on a running production server.

That is important, and that make the separation from good code and production worthy code.

Comments

mohammad
09/11/2012 10:27 AM by
mohammad

Good post!

Craig
09/11/2012 10:48 AM by
Craig

I think Spolsky also makes a good argument in that article on why rewriting to clean up code is bad. Because you are throwing out all those hard to find and fix bugs. You get cleaner code, but less stable system.

Doron
09/11/2012 11:12 AM by
Doron

I disagree. Professional code should be both clean and production ready. "It works" is a pretty bad excuse for a class that has thousands lines of code. It's probably not fun to make changes to that class.

Ayende Rahien
09/11/2012 11:47 AM by
Ayende Rahien

Doron, You usually can't have both.

In particular, error handling is generally ugly code, and at low levels, you have a LOT of it.

Paul Shmakov
09/11/2012 12:22 PM by
Paul Shmakov

"Release It" by Michael Nygard is a good book on making software production ready.

njy
09/11/2012 12:31 PM by
njy

@Oren: not to start a flame or something, but don't you think that what you are saying right here is in a kind of conflict with your usual Ayende-style code reviews?

Things like "oh look at this crappy piece of code, it's unreadable!" or "it's not understandable, and this is important because code is written once and read a thousand times" or other stuff like that.

I mean, i would simply like to understand the different approach, because otherwise it may seems something like "do what i say, not what i do" and the likes.

Waddaya think?

Ayende Rahien
09/11/2012 01:01 PM by
Ayende Rahien

Njy, Most of the code that I review is a pretty high level one. There are different set of considerations for the type of code that one write. And there are different sets of problems.

njy
09/11/2012 01:05 PM by
njy

@Oren: ok, so we may say that if the code is low level it is acceptable to have some sort of "roughness", and instead if it is high level it should be more readable, concise and overall understandable? I'm just trying to have a clearer view on your opinions about code quality, not to nitpick here and there ;)

btw thanks

Daniel Lang
09/11/2012 01:47 PM by
Daniel Lang

I couldn't agree more. I made the mistake twice and replaced old and awful code with fresh and clean code. In the end, I ended up fixing the same bugs twice and even worse, I didn't have a working release for a long time.

I learned two things: 1) Make sure you always have a working release that is not older than a week. 2) Only throw out things that are small enough that you can replace them within a day or at most two.

Alwin
09/11/2012 02:00 PM by
Alwin

njy: I think there's a difference in wrong use of frameworks and libraries, NIH syndrome etc (code reviews by Ayende) and code that started with a good design but grew out of control because of bug fixes and error handling (this post).

Ayende Rahien
09/11/2012 02:03 PM by
Ayende Rahien

nNjy, Yessss... although I am not quite sure that rough is the right word. If you'll look at my code reviews, I rarely talk about things like Cyclmatic Complexity, naming conventions or things like that. When I criticize code it is usually because it is doing things wrong. Off the top of my head, the wrong things are usually: * Needless abstractions - controller -> service -> repository * Violation of known fallacies - select n+1, unbounded result sets, assuming no bandwidth / latency. * Bad error handling.

The details of what constitute a problem may differ between layers of code, but the actual problems remain the same.

Ayende Rahien
09/11/2012 02:04 PM by
Ayende Rahien

Alwin, It isn't out of control. Ugly code is still pretty good code. But when you have to take into account error handling in your error handling code, that gets a bit messy.

Josh Kodroff
09/11/2012 02:24 PM by
Josh Kodroff

I think you can have it both ways.

As long as you keep your functions short and well-named, you can have readable code with all the necessary error handling. Just keep the error handling in separate functions.

Alwin
09/11/2012 02:37 PM by
Alwin

Ayende: yes i used the wrong words there, I meant "started with a good design but grew ugly". But I only tried to explain the difference.


I also think that documentation is important: why is this code so ugly? Otherwise the next dev will just think "WTF?" and "clean up" everything. Documentation like good function names (self documenting code) or maybe comments that reference issue numbers.

Ayende Rahien
09/11/2012 02:40 PM by
Ayende Rahien

Josh, You can't really do that.

For example, let us take the case of handling an HTTP request, okay?

  • Get context
  • Dispatch request
  • Flush request

Now, in any part here you may have errors, during the error handling, you may have errors as well. You need to handle all of that.

Typical example, during the request processing, we threw an exception. We need to send the error to the client. But we get an error there because they disconnected.

Raciel
09/11/2012 02:43 PM by
Raciel

As everything in code, "it depends". Whenever I have started a new application it is my goal, and I put a lot of effort on it, to keep it simple, clean, readable, maintainable, etc... As soon as the systems matures, it gets dirty, that type of dirt you don't like but you need to live with because "it works" and because -honestly- you can't make it better; it is simply ugly. I have worked on several production systems, and I have failed miserably in a lot of reworks attempts. The older I get, the more I'm for the principle "why to fix/re-factor if not broken". Specially because there is a high cost (time, money, customer confidence lost when old-well-known-bugs arose) associated with it.

Doron
09/11/2012 03:05 PM by
Doron

Oren, Are you really saying you can't make that 1K lines class more elegant by splitting it to more classes that do less? Yes, error handling can get messy but you can put the messiness in smaller packages that are easier to work with.

Looking at the code here: https://github.com/ravendb/ravendb/blob/master/Raven.Database/Server/HttpServer.cs There are lots of things you can put in another class, like all the Handle*Exception methods. (Although I must admit that as far as huge classes go, this is pretty clean).

JV
09/11/2012 03:15 PM by
JV

'About the most unprofessional thing that you can say is: “I have no idea what is going on.”'

Hmm, I often say this when interacting with the business people, though.

LOL, I just had to say it. :)

Ayende Rahien
09/11/2012 08:15 PM by
Ayende Rahien

Doron, Sure, I can move the code to a helper class. What did I gain by that? The code is NOT reusable, I have merely made it harder to understand by removing it from its usage location.

Doron
09/12/2012 06:18 AM by
Doron

I think the code would be clearer as it would have less clutter. Small classes that do just one thing are easier to work with and evolve, regardless of reusability. Don't you agree?

Ayende Rahien
09/12/2012 06:53 AM by
Ayende Rahien

Doron, If it is detached from context, it is going to make it harder to figure out.

tames
09/12/2012 12:50 PM by
tames

While I agree that "professional code" is something that is maintainable... the idea of keeping layers upon layers of technical debt just to avoid losing the dozens of bug fixes is a mistake. Time and again I've faced the issue of legacy code that was "untouchable" because they didn't want to lose the "knowledge" embedded in the code. I usually reply that if you don't know what it is that's embedded, then that knowledge is already lost.

Refactoring is an important part of the life of a codebase. Each of those little bug fixes should have been accompanied by a change to the unit tests. Those unit tests will help you ensure that the refactored code still works as expected, but is made cleaner... and clearer... for the next developer.

Aaron
09/12/2012 03:02 PM by
Aaron

Great post -- and rightly points out the importance of production readiness in an application. Code should be both Clean and Production Ready and it is almost always possible.

Some would argue you can't have both -- and they are right in some cases -- but like most software design problems, abstraction is the answer.

I subscribe to clean code wholeheartedly but the main point that many "clean code" zealots miss is that the writing of the code occurs in a small percentage of the overall lifetime of an application. Fixing that code and adding incremental changes takes up the vast majority of the time so code should be designed considering that. Another large percentage of an applications "interaction lifetime" is support -- identifying problems. So builders of such a system should design with that in mind and build tools that enable support staff and even users to discover their problem and report details for help.

None of that requires "dirty code" -- that's a myth. Dirty code may be more expedient or business appropriate and if we're honest about it we can accept that. But most dirty code problems can be solved by better code and/or abstractions -- albeit less expedient.

Clean code AND Production Ready code is possible and usually practical... it just might take more investment upfront.

Karep
09/12/2012 08:52 PM by
Karep

Did I just find bug in HttpServer.cs in line 114?

Ayende Rahien
09/13/2012 05:30 AM by
Ayende Rahien

Karep, It is possible, what did you find?

Karep
09/13/2012 09:09 AM by
Karep

if (TryParse(configSetting, out val)) val = 60

Shouldn't it be if (!TryParse) ?

Ayende Rahien
09/13/2012 09:40 AM by
Ayende Rahien

Karep, You missed the == false in the end.

Karep
09/13/2012 09:42 AM by
Karep

Not in line 114, in line 111 there is == false.

https://github.com/ravendb/ravendb/blob/master/Raven.Database/Server/HttpServer.cs

Ayende Rahien
09/13/2012 09:44 AM by
Ayende Rahien

Karep, Scroll to the side in line 114, the == is there.

Karep
09/13/2012 08:10 PM by
Karep

Ok, I did not find a bug :)

Moses
09/15/2012 03:29 PM by
Moses

You found Frequency misspelled :) FrequnecyToCheckForIdleDatabases

Comments have been closed on this topic.