Soapbox Rants and Raves

I have spent a lot of time discussing software principals and architecture over that last couple of years, and the subject of error handling and recovery always seems to always rear its ugly head. Most recently, in talking about moving from monolithic application deployments to a distributed, web service orientation, many developers seem to immediately go into paranoia mode. They only see two choices for working with distributed systems: the two phase commit or store everything approach, such that it can be resubmitted when the service is available. There is one other option which we cannot forget about, used by many applications regardless of their deployment, the traditional “logging, debug, retry” logic. Unfortunately, we developers like to add this code at the end of the development cycle, and usually as an after thought; but more on that later!

Let’s talk reality… When was the last time one of your systems actually failed due to some type of hardware or service failure? I honestly don’t recall a single failure. Considering today’s redundant hardware technologies, I believe it would be pretty hard to have a total system failure; provided we move away from those monolithic application deployments! I agree that some kind of isolation layer (protection) is required for systems outside of our operational control, such as remote web service. But, our internal network and applications should be a different discussion, one that assumes 100% availability. I realize that some systems can experience performance degradations, but that is also a blog for another day, not something that we should be “protecting” ourselves from. I believe that most of the bugs we encounter in the production environment are actually caused by incorrect or misunderstood requirements, and the the occasional developer contribution  (bad code, insufficient testing, etc.), not by a system failure. Another interesting finding, many of the bugs are actually found in the logging and monitoring (prevention) code, rather than the true business logic. What or who are we actually protecting ourself from?

One of our top goals should be to produce the smallest, simplest amount of code possible to implement the solution. I feel there are two (2) fundamental problem types:

• Show Stoppers. A show stopper is a dependency in which your system cannot perform its primary function. For example if you created a purchasing system that relied on an external pricing service, your system cannot function without live prices. Assuming that this pricing service is internal to your company, there is no need to protect your service/application from this scenario. The actual risk of this type of failure is not worth the amount of effort required to implement and maintain a possible fall-back solution.

• Recoverable errors. Recoverable errors are software problems that can be captured and dealt with later, at an appropriate time. Examples might include a failed database update or the failure to update a logging service. If the transaction can be still be completed, without the end user being impacted, this is an acceptable approach. The assumption is that we don’t have tens or hundreds of this issues to deal with, as they can and will happen over time. We just need to ensure that we have a method for recovering from the problems, ensuring that the system can be put back in the proper state of integrity.

I think the  relevance of this post is to ensure that “software issues” can be detected. If we can assume that collaborating internal systems are always available, we can eliminate most of the code to manage these dependencies. We should focus on a  common design/implementation/strategy that can ensure the detection of issues. Looking across existing applications, it seems that each one has its own unique way of capturing and monitoring problems; with each team spending significant design and implementation effort attempting to create a robust method. Ironically, with all of the work developers do, many of these systems have problems that go undetected for hours,days and even weeks, without anyone ever knowing that an “issue” happened. It appears that we should spend more effort on a common monitoring strategy to detect problems; one that makes us aware of problems within minutes, not hours of the problem occurrence.

The second part of this approach is the ability to recover. There are probably some other important concepts that I have in my head, not part of this blog, which actually makes this all possible. I think for a system to be recoverable, it should  be compartmentalized; such as smaller databases, each with its own functional domain areas; rather than a single, gigantic database with full referential integrity. By breaking the system (services and database) into smaller, self-contained “mini” environments, each detected failure has a smaller overall impact on the system, and ultimately makes it easier to recover, as the impact is now isolated to a distinct area of the system.

Hopefully, with ability to detect problems earlier, minimizing their impact, and simplifying the recovery process, should make the prevention of the problem easier to address. With smaller, simpler systems (services), it will be easier to locate and correct the offending code (because there is less code and data coupling), creating less risky changes which are easier to test and validate. Sounds like there is an overall win-win architecture in here somewhere… I sure hope I get to build a system with these principals and goals some day!!!!!

When you start researching the code review process, you run across a lot of interesting blog postings, from the process perspective to the emotional aspects.  I found this one, Working with your angry coder, and thought it was worth sharing. I stole the picture from his site too; both the kids and I thought it was funny!

Over that last year, I have definitely tried to be more aware of the comments I make. However, I do remember saying this phrase numerous times in the past “Whoever wrote this code should be shot!”.  I too apologize for to all of the people I’m made cry over the years! I believe that most of these comments were directed at the code and never the original author; I don’t think I am that angry!  I do have a passion and strive for perfection (at least the best job that I can do, given the scenario), which makes it very easy for me to be perceived as an angry coder. I used to think that I was the only one who cared about developing  software quality. Someone told me that most developers are generally passionate about something in the process, usually just not from the same perceptive as me. So in reality, everyone is trying to do the best they can, we are all just coming at it from different directions. We just have to find a way to work more smoothly as a team. As one of the comments said, it is all about understanding each other. To bring this back to the code review process, I think it is all about finding bugs and learning from each other.  If we truly practiced collective code ownership, maybe none of us would take the comments so personally and we would have less anger!

I recently did an impromptu survey of some friends and coworkers concerning how they conduct code reviews. The responses I got back were a little surprising and definitely not what I was hoping for. None of the responses were incorrect or wrong, but I was hoping that my vision and theirs would be a little more aligned.

Like many of you, I have been involved in my fair share of code reviews. Unfortunately, most of these quote code reviews are more of a code presentation, rather than an actual review. The presenters typically don’t prepare anything for the reviewers, and if they do, the reviewers never have the time to study or evaluate the material. Once the code has been reviewed, you have the challenge of dealing with the  issues generated by the reviewers: How are the issues being tracked? Is there a discussion or evaluation of each issue? How are the issues being prioritized? When and how will each issue be fixed? Don’t forget about the general coordination and procedural aspects: Who is doing the coordination? What code is being reviewed? Who are the scribes? What is the appropriate tracking or reporting format? This is all part of the traditional code review process, or so I thought!

The most interesting response from my survey was that many people considered my definition of a code review to be rather old school and non productive.  I’m not old school, just getting older and hopefully wiser! Several teams said they used tools like Checkstyle and PMD to verify the code, combined with Agile and Pair Programming techniques; thus minimizing (but not eliminating) the need for an actual code review. Teams that conducted reviews seemed to prefer the Net-Meeting (a shared desktop) approach, over meeting together in a conference room. Net-Meetings can be valuable, but I believe that most people are either surfing or reading emails and generally not paying attention during the meeting. Some teams even fix issues right there in the code review. Personally, I think this is very bad practice and not a good use of everyone’s time. Up to this point in the process, I’m pretty much on board with everyone’s process, but I think there is too much informality and and not enough traceability in the overall process. I don’t want to add more bureaucracy, I actually want to make the process more efficient and effective.

I believe in the “pair programming” principal, but consider “pair designing” much more valuable. Teams could minimally implement (and should) some type of virtual partners to get the benefits of pair programming. I like working on projects where some of the team members monitor all of the check-ins, basically performing continual code reviews throughout the development process.  I realize code reviews can have other potential values, such as cross training, mentoring, and promoting consistency and reuse. However, I don’t believe this is the purpose of a code review; isn’t it really about identifying programming flaws? So the question is, what is the most effective way to find bugs?

Some code reviews focus too much on the fluff. Are the indentations and parenthesis in the right place? Are the classes, properties, and variables named appropriately? Is the code well structured? All important aspects, but really don’t need to be addressed in a code review process. All of these issues should have been identified during construction phase using freely available tools, integrated with the IDE and the Continuous Integration (CI) process. I have been using the Checkstyle and PMD for many years to accomplish this task. They can provide value when the team requires the violations to be fixed. I typically configure the Eclipse plug-ins and CI tool to fail if any violations are encountered. Additionally, the CI tool will continue to run, spamming the contributors until the issues are resolved. I have the same philosophy for failed jUnit tests too, I run a tight ship! When violations or jUnit failures are flagged as warnings, rather than errors, they typically get ignored by the developers and are never addressed. When used correctly, Checkstyle and PMD will encourage and enforce specific coding practices, acting as a virtual coding partner, identifying issues when they are created.  Personally, I think these tools help me write better code; as the code evolves and begins to deviate from the original design, these tools usually pick up on the “band-aids” and allow me to fix/adjust the design before it is too late.  I recently added two more tools to my quality validation toolkit, Findbugs and the Testability Explorer.  There is some functional overlap in these tools, but I still like using all of them as a suite, as they each do their job a little differently. Findbugs is very interesting, it works from the compiled byte code, rather than the source code. Findbugs analyzes your code and its interaction with the other included libraries. It will detect problems such as unclosed resources, synchronization issues, and returning mutable collections. The Testability Explorer is my current favorite, but not as easy to understand as the other tools. I will write about it in another post.

Let’s say that we have we have these tools in place and our code is super clean. Unless projects implemented pair programming and are skipping the code review process, most schedule some type of review at the end of the coding cycle. I think most teams identify a random subset of code and schedule a series of meetings to conduct the reviews.  I say random because the developers pick and choose what files are to be reviewed.  How can we be sure that all of the important changes have been identified? This is where each team charts their own path. Some teams have a code “presentation” session with no pre-meeting preparation what so ever. Other teams email out a source file or URL list for each file to review. On the last code review that I helped coordinate, we sent out FishEye URLs for each file to the reviewers. This was pretty cool, because the URL represented the actual changes, not simply a link to the file. This allowed the reviewer to focus on the changes, rather than the entire file. This is point where I believe the whole processes breaks down, how do you report changes back to the developer and track the resolution? Usually issues and comments are sent back in email format, added to a spreadsheet, and checked into version control. One team actually generated PDF versions of their source code and mailed that out, the reviewers then added annotations to the PDF for their comments; kind of interesting. At our code review meetings, we tried to walk through the email comments and tried to address each one. This worked pretty well for those that actually prepared, but we still had lots of manual follow up work.

What I dislike most about this process is the management of these problems. Wouldn’t it be interesting if each issue had its own mini-workflow and was stored in some kind of relational database. Ideally, some kind of Eclipse plug-in to manage them, not necessarily linking the issue directly to the source code, but giving the developer quick access to the problem and approximate location within the file. The issues could easily be easily categorized by type and severity, and managed by the development team, and ultimately reported to management.  There are numerous commercial tools (SmartBear, Atlassian)  and even open source tools (CodeStriker, Rietveld, Review Board) which will automate much of the review process. These tools are actually very cool. It would be so efficient to review, debate, and resolve each issue in an on-line tool,  looking at the actuals changes; just click on the line where the problem exists and create a new comment (start a workflow). The biggest problem with many of these open source tools is around deployment. Many corporations have rules and processes for bringing in open-source tools.  Most of these tools have tons of required components, just to make them work in the environment. Its not like I can just log on to one of the UNIX servers and do a bunch of apt-gets to install the software. Review Board for example, requires: python-setuptools, memcached, libmemcache-dev, Djblets, Django-Evolution, Django, flup, and Python Imaging Library, plus integration with a database. Oh, guess what… it does not support Sybase or Oracle!  Why can’t these guys just give me WAR file that I can drop into an Tomcat server or better yet, have it integrated with Jetty so I can just fire it up!

Even though I appear to be on my own code review island, I will keep monitoring this tools for one that can easily be integrated into the corporate environment. I just might have to build it myself!