+972 52-548-6969
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
Good post!
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.
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.
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.
"Release It" by Michael Nygard is a good book on making software production ready.
@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?
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.
@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
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.
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).
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.
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.
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.
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.
Josh, You can't really do that.
For example, let us take the case of handling an HTTP request, okay?
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.
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.
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).
'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. :)
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.
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?
Doron, If it is detached from context, it is going to make it harder to figure out.
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.
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.
Did I just find bug in HttpServer.cs in line 114?
Karep, It is possible, what did you find?
if (TryParse(configSetting, out val)) val = 60
Shouldn't it be if (!TryParse) ?
Karep, You missed the == false in the end.
Not in line 114, in line 111 there is == false.
https://github.com/ravendb/ravendb/blob/master/Raven.Database/Server/HttpServer.cs
Karep, Scroll to the side in line 114, the == is there.
Ok, I did not find a bug :)
You found Frequency misspelled :) FrequnecyToCheckForIdleDatabases
Comment preview