Soapbox Rants and Raves

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!

I recently wrote a post about how Eclipse helps to maintain the developer’s focus; what I really wanted to write about was the Mylyn plug-in.  I recently worked on a small project where we setup a completely open-source development environment, built on top of an Ubuntu Linux server. What made the environment interesting, was how well everything worked together. One of the more challenging issues working on a small project is the communication and task management; this is where Mylyn helps smooth out the process.

Mylyn is a simple Eclipse plug-in that integrates with a large number of commercial and open-source issue tracking systems. On the surface, it looks like any other tracking tool, but provides far more functionality than a simple web-based solution.  Building on my previous post, Emacs to Eclipse, the developer can conveniently manage their programming assignments without leaving Eclipse. It even has a notification system which alerts the developer to newly created or assigned tasks. I believe this approach is far more effective than relying on email or external applications to manage the development process.

What makes Mylyn unique, is that each task can be assigned a context. The context represents the state of the Eclipse session, maintaining a list of all of open files. The first step is for the developer to activate a task. Once activated, the developer has the ability to assign a context to that task.  From this point forward, as long as that task is active, the context will be kept in sync with all of the files that the developer opens.  If the developer chooses to activate a different task, the current task is deactivated and the new task is activated. The open files are automatically closed and the files linked with the new task are opened; even restoring the cursor position within the files.  The is extremely helpful when working on multiple, unrelated tasks which are being worked in parallel.

There are a variety of other subtle integrations that also improve the developer’s effectiveness:

• One of the more convenient features is the ability to create tasks from any “warning” or “TODO” comment. The developer just right clicks on the marker to generate the new task.
• Mylyn is also integrated with the Team Plug-in (SVN/CVS). It will automatically populate the commit message with the task information.
• Using the plug-in’s file upload functionality,  developers can easily attach supporting documentation (stack traces or generated files). There is even a built-in screen capture utility which allows the developer to grab, crop, and annotate images, which can be quickly attached to the task.
• Mylyn magically tracks the amount of time the developer spends working on a task. It appears to be the real time spent on the ticket (based on having Eclipse open and active, not simply based on when the ticket was opened and closed; pretty cool!). This could be very useful in Agile projects, comparing the estimated time to the actual time spent on the task. What a great way to help the developer to improve their future estimations.

The primary issue tracking tool where I work does not integrate with Mylyn. It would be nice if it did, but I don’t know how important that really is. I have been thinking about the integration of a secondary, fine grain task management tool into the environment that is more developer focused.  I actually think it would be interesting to have two issue tracking systems. The first system would be exclusively for the QA and production support teams, mainly to track defects discovered in testing or production related issues.  The second system would be used exclusively to manage the development process. Why create tens or hundreds of tickets in the real ticket tracking system, just for the development team to build the next release? The answer really depends on the scope or purpose of the ticket, but generally it is too much work to create task level assignments in real tracking system. I would really like the tickets in the development environment be more fluid and granular, actually representing the work that needs to be done, not an outdated, vague interpretation of what might happen during development cycle. If a create needs to be created for a developer to refactor a method or add a jUnit, no big deal.  If I’m using the real production ticketing system, I would need several levels of approvals, both from the business sponsor and test teams. Unfortunately, this impledes progress and the actual work being performed is of no interest to them anyway, it is just part of the normal development process. The bottom line is that I currently have no way to easily task or manage the development process or myself; it is currently done through email chains and some crude “TODO” comments in the code. Kind of a bummer when the technology exists, for free (minimal configuration time) and could actually be a huge benefit for the development teams. If you include the planning aspects of the tool, I could see this being a huge help for the project managers as well.

Unfortunately, because of the infrastructure requirements (mysql, tomcat, etc.) of the open source tools (bugzilla, trac, etc.), it is politically too difficult for teams to integrate in the corporate environment. I recently read about an interesting Mylyn connects that lets you store the tracking information in a relational database, rather than a full featured ticket tracking system. I have yet to explore this option, but it might open up the door for allowing me to utilize Mylyn, without the infrastructural overhead. I will keep you posted!

A recent manager of mine worked tirelessly to drill this concept into our team: Build, Package, Deploy. If you step back and think, it is a very simple, yet powerful paradigm. I work in an environment that has not fundamentally changed the way we deploy J2EE applications for at least, I am only guessing here, but could be close to 10 years. The group that managed the Internet defined a standard footprint, multi-site J2EE cluster, with all components (Servlet, EJB, etc.) deployed on the same instance. Everyone was given the same configuration and we ended up building a large collection of heavy, monolithic applications. The worst part was that each application was dependent on the specific J2EE container and hardwired to the deployment environment, making an extremely static, inflexible, unscalable solution. Many of us developer’s silently rebelled against the J2EE bandwagon and built simple Servlet-based applications; making sure to use as little as possible, from the provided J2EE container (no connection pools, or even JMS). We pushed to use a simple Servlet container, such as Tomcat or Jetty, but our pleas went unanswered. The default architecture was structured in such a way, that everyone created huge, highly coupled, monolithic web applications; a single WAR that did everything, from vending JSPs, to performing business functions, to accessing the database. Unfortunately, I too was brainwashed by the architecture and stopped thinking about how architecture could be done differently. Fortunately for me, that recent manager was new to the company and had completely different ideas about architecture. Instead of starting a revolution, we embarked on a evolutionary journey to change the way we build software, rather than focusing on how the systems were deployed. One of the first points of enlightenment, to change the way we thought about building system: build it, package it, and then deploy it.

I will tie this paradigm back to the Lean principle of postponing decisions (committing to something) until you actually have to make the decision (the commitment). How does this apply here? When we build applications today, the developers know up front, where and how the system (code) will be deployed. The developer will make design decisions, many of which are unconscious, based on their knowledge of the deployment environment; hardwiring their implementation for what the application will look like when it is deployment. Without giving this much thought, it almost sounds like the right process. Unfortunately, we probably just boxed ourselves right into a corner!

Would it not be better, if the developers simply built components, the true business functionality, without thinking about how the software would ultimately be packaged or deployed? Obviously, there would be some architectural patterns to be followed, but the developer’s would have no idea how their component will be assembled into the final application. Now let’s imagine that we have a Component Packaging Specialist; they would be responsible for bundling up components into deployable packages. This person determines the optimal combination of components, creating a virtual application that performs the required business functionality. This virtual application is bundled in such a way as to create minimal dependencies between bundles, which are independently migrate-able and allow high scalability. Finally, the bundles are given over to a Deployment Specialist, who figures out how best to deploy the virtual application into the environment. Depending on the operational requirements, the virtual application could be deployed in any J2EE or simple Servlet container. How many of each bundles are to be deployed would be determined by the predicted transaction volumes. If the actual volume exceeds the expectations, the Deployment Specialist can easily adjust the environment to accommodate the volumes, without involving the development staff.

The primary point is to allow as much deployment flexibility as we can afford, by not letting the code dictate how or where the application will be deployed. This approach provides less risk (no need to change code) and creates fewer dependencies (better portability). Developers should remember and separate these three tasks as they design and implement systems, even if one person is performing all three roles. By separating these three tasks (actual disciplines), we should end up with better systems, providing the operational flexibility to meet the ever changing business demands.

I found the blog of Miško Hevery last week; he is an Agile Coach at Googe. Scanning his recent posts, he seemed to capture many of my own thoughts, saving me the time of actually having to put them into words! Please check out his posts, topics range from testability, to dependency injection, to Changing Developer Behavior(1)(2).

His blogroll lead me to some other great sites: The Agile Buddy and Agile Journal. I have yet to read these two posts, but they really grabbed my attention:

• Doing the Right thing vs Doing it Right
• Requirements Come Second

I’m sure that I will enjoy this read, as the second paragraph of the Requirements Come Second post states that  making the development environment more effective should be the first priority.  I have been a huge advocate of creating an effective development within teams for a long time. I typically feel that I’m on an island by myself, as most people don’t really care what or how something gets done, as long as the date is not missed.  Just getting it done has never been good enough for me.  Obviously, projects have to get done or none of us would have jobs. So the question is, why can’t we get the project and and do the right thing?  Hopefully, there will be some good wisdom in the post! Enjoy.

I’m currently reading the Lean Software Development book, it is so interesting… such a different way of thinking and actually “accomplishing” work….    I can only hope that the projects I am currently supporting will move to this style of software development some day….  I once interviewed at a company that fully embraced the XP methodology… it was so cool (from the wall-sized picture of the project, to the story board, to the standing meeting, so progressive), it actually started my evangelism of this process at my current employer.  I can’t recommend this book enough, especially if you want to see a contrast to traditional software development, and hopefully you guys will then be drinking the same kool-aid as me!

Anyway, I was reading a magazine this morning, and to my surprise there was an article on Kanban; which I was just reading about last night.   The article also talked about a piece of software, which I thought was interesting; having software to help manage a lean/agile project looked very interesting… We have used the office windows as our board on previous projects, not nearly as cool. Even Source Forge has a Kaban tool.

I just think it is a neat way of “depicting” and “choosing” the work that needs to be done… It is a very different way of thinking and managing a project.

This is probably common knowledge, but I think is root cause is generally overlooked and many times we just don’t care what is really causing the problem, just how quickly can we fix it…  I recently read this in the Lean Software Development book, and thought it was really interesting and simple; something that I should focus on more… So, I started Googling the 5 Whys this morning and found out a couple of interesting things…  This approach was first used (or documented) by Toyota, which much of the Lean book is based upon. What I did not realize was that is now rolled into Six Sigma.  Here is a quick Wiki page you can read too…  I just think this is something we should all keep in our problem solving toolkit, it never hurts to ask!