http://ayende.comAyende @ RahienCopyright (C) Ayende Rahien 2004 - 2021 (c) 202660You found Frequency misspelled :) FrequnecyToCheckForIdleDatabases http://ayende.com/158369/on-professional-code#comment31http://ayende.com/158369/on-professional-code#comment31Sat, 15 Sep 2012 15:29:44 GMTOk, I did not find a bug :) http://ayende.com/158369/on-professional-code#comment30http://ayende.com/158369/on-professional-code#comment30Thu, 13 Sep 2012 20:10:24 GMTKarep, Scroll to the side in line 114, the == is there.http://ayende.com/158369/on-professional-code#comment29http://ayende.com/158369/on-professional-code#comment29Thu, 13 Sep 2012 09:44:17 GMTNot in line 114, in line 111 there is == false. https://github.com/ravendb/ravendb/blob/master/Raven.Database/Server/HttpServer.cshttp://ayende.com/158369/on-professional-code#comment28http://ayende.com/158369/on-professional-code#comment28Thu, 13 Sep 2012 09:42:59 GMTKarep, You missed the == false in the end.http://ayende.com/158369/on-professional-code#comment27http://ayende.com/158369/on-professional-code#comment27Thu, 13 Sep 2012 09:40:46 GMTif (TryParse(configSetting, out val)) val = 60 Shouldn't it be if (!TryParse) ?http://ayende.com/158369/on-professional-code#comment26http://ayende.com/158369/on-professional-code#comment26Thu, 13 Sep 2012 09:09:26 GMTKarep, It is possible, what did you find?http://ayende.com/158369/on-professional-code#comment25http://ayende.com/158369/on-professional-code#comment25Thu, 13 Sep 2012 05:30:31 GMTDid I just find bug in HttpServer.cs in line 114?http://ayende.com/158369/on-professional-code#comment24http://ayende.com/158369/on-professional-code#comment24Wed, 12 Sep 2012 20:52:34 GMTGreat 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.http://ayende.com/158369/on-professional-code#comment23http://ayende.com/158369/on-professional-code#comment23Wed, 12 Sep 2012 15:02:23 GMTWhile 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.http://ayende.com/158369/on-professional-code#comment22http://ayende.com/158369/on-professional-code#comment22Wed, 12 Sep 2012 12:50:52 GMTDoron, If it is detached from context, it is going to make it harder to figure out.http://ayende.com/158369/on-professional-code#comment21http://ayende.com/158369/on-professional-code#comment21Wed, 12 Sep 2012 06:53:20 GMTI 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?http://ayende.com/158369/on-professional-code#comment20http://ayende.com/158369/on-professional-code#comment20Wed, 12 Sep 2012 06:18:33 GMTDoron, 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.http://ayende.com/158369/on-professional-code#comment19http://ayende.com/158369/on-professional-code#comment19Tue, 11 Sep 2012 20:15:48 GMT'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. :)http://ayende.com/158369/on-professional-code#comment18http://ayende.com/158369/on-professional-code#comment18Tue, 11 Sep 2012 15:15:01 GMTOren, 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). http://ayende.com/158369/on-professional-code#comment17http://ayende.com/158369/on-professional-code#comment17Tue, 11 Sep 2012 15:05:07 GMTAs 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. http://ayende.com/158369/on-professional-code#comment16http://ayende.com/158369/on-professional-code#comment16Tue, 11 Sep 2012 14:43:57 GMTJosh, 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. http://ayende.com/158369/on-professional-code#comment15http://ayende.com/158369/on-professional-code#comment15Tue, 11 Sep 2012 14:40:14 GMTAyende: 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.http://ayende.com/158369/on-professional-code#comment14http://ayende.com/158369/on-professional-code#comment14Tue, 11 Sep 2012 14:37:51 GMTI 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.http://ayende.com/158369/on-professional-code#comment13http://ayende.com/158369/on-professional-code#comment13Tue, 11 Sep 2012 14:24:12 GMTAlwin, 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.http://ayende.com/158369/on-professional-code#comment12http://ayende.com/158369/on-professional-code#comment12Tue, 11 Sep 2012 14:04:19 GMTnNjy, 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.http://ayende.com/158369/on-professional-code#comment11http://ayende.com/158369/on-professional-code#comment11Tue, 11 Sep 2012 14:03:28 GMTnjy: 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).http://ayende.com/158369/on-professional-code#comment10http://ayende.com/158369/on-professional-code#comment10Tue, 11 Sep 2012 14:00:39 GMTI 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. http://ayende.com/158369/on-professional-code#comment9http://ayende.com/158369/on-professional-code#comment9Tue, 11 Sep 2012 13:47:49 GMT@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 thankshttp://ayende.com/158369/on-professional-code#comment8http://ayende.com/158369/on-professional-code#comment8Tue, 11 Sep 2012 13:05:27 GMTNjy, 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. http://ayende.com/158369/on-professional-code#comment7http://ayende.com/158369/on-professional-code#comment7Tue, 11 Sep 2012 13:01:59 GMT@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?http://ayende.com/158369/on-professional-code#comment6http://ayende.com/158369/on-professional-code#comment6Tue, 11 Sep 2012 12:31:34 GMT"Release It" by Michael Nygard is a good book on making software production ready. http://ayende.com/158369/on-professional-code#comment5http://ayende.com/158369/on-professional-code#comment5Tue, 11 Sep 2012 12:22:45 GMTDoron, You usually can't have both. In particular, error handling is generally ugly code, and at low levels, you have a LOT of it.http://ayende.com/158369/on-professional-code#comment4http://ayende.com/158369/on-professional-code#comment4Tue, 11 Sep 2012 11:47:21 GMTI 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.http://ayende.com/158369/on-professional-code#comment3http://ayende.com/158369/on-professional-code#comment3Tue, 11 Sep 2012 11:12:18 GMTI 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.http://ayende.com/158369/on-professional-code#comment2http://ayende.com/158369/on-professional-code#comment2Tue, 11 Sep 2012 10:48:16 GMT