Whether by personality or just experience I’ve always leaned towards decision making by consensus. Most recently, I’ve been employing the consensus approach with 5 of our senior java developers working to define our development guidelines and practices. That means we don’t have some really exact list of standards, but where we do come to agreement the guideline is a lot more enforceable than one decided strictly by management or a single architect. Even worse an architecture consultant selected by management who never has to deal with the results of their mandated ‘best practices’. Anyway apparently I’m not alone as this is the policy at Google:
Strive to reach consensus. Modern corporate mythology has the unique decision maker as hero. We adhere to the view that the “many are smarter than the few,” and solicit a broad base of views before reaching any decision. At Google, the role of the manager is that of an aggregator of viewpoints, not the dictator of decisions. Building a consensus sometimes takes longer, but always produces a more committed team and better decisions
Probably explains why they have three major languages in C++, Python, and Java instead of insisting on a single one.
I hold a semi-regular development practices meeting where we cover all sorts of topics to get to an eventual consensus among our senior developers. The topics range from using maven versus ant to where to put curly braces. A few managers are attending now including a VP, but it hasn’t seemed to dumb down the dialogue yet.
Today we decided a few things:
- We’re going to continue looking at maven on new projects.
- We are holding to our current coding standard with curly braces starting on the same line as the declaration and the ending bracket on its own line. (We did have an impassioned argument to the contrary from an experienced Delphi developer who’s very used to the starting bracket on a new line. His compromise is that he can format the code the other way and then just reformat before checking in to CVS.)
- We need to decide on a central wiki.
We actually have a wiki solution for putting up developer documentation about admin and configuration tasks with JBoss or Websphere. One of our developers got the wiki bug and got JSPWiki installed and running for our group. There was a lot of activity at first and then it seemed the wiki was in danger of drying up and dying. I’ve seen this happen before, but recently just this week two of my developers decided to put some new workaround tricks up on the wiki.
In the meeting one of the developers described how it had been painful in the wiki to add a code sample since they ended up having to insert non-breaking spaces on each line to indent the code. So if we pick a new one they really want it to be easier to use, preferably with a semi WYSIWYG editor. And they really want it to be nicer looking and easy to break up by projects.
The output of this was I ended up with an action item to look into Confluence. After dinking around with the demo site, I think it fits the bill pretty well, especially with the new semi-WYSIWYG interface they’ve added for the 2.0 release. And it has tons of separate spaces that you can create for anything you want. So I’ll probably install the 30-day trial next week.
We finally held our first code review today after having planned them more than a month ago. It was important enough to me to ignore my current stomach flu and attend the code review anyway.
The review involved myself, the tech lead on the project, and one senior developer. I’d reviewed the process with the tech lead, but as these things go some things fell through the cracks, like that there should be two days to review the code before holding the review. Anyway it was more important that we got started.
We met in the team room which was pretty much my office until about 2 weeks ago when I moved out to cubicle land. The tech lead had prepared printouts that he handed out the day before with 5 classes for the review. I setup a projector so we could jump through the code on the overhead as well and we recorded the mandatory and optional changes on the whiteboard.
I ended up leading the review process, probably because this is more of my initiative than anyone else’s. The senior developer admitted he had never gone through a code review before and was interested in seeing how they worked. Out of the five classes being reviewed most of the time was spent on a single JSF backing bean class.
To start out with I did a quick review of the process checklist (see PDF version). It’s still in rough form and bound to get cleaned up, but it covers pretty much our initial process. I asked if either of them had run a static checker like Checkstyle, or the code analysis tools in RAD or IDEA. Neither of them had, so I mentioned I had at least looked at Checkstyle and showed them the output on the latest email from a cruisecontrol build. They seemed to think it would still be useful to run the check, they just didn’t get around to it this time.
After that I started going through the first large class where we ended up spending most of our time discussing. This was an interesting class to review since it contained a mix of RAD JSF generated code and the developer’s actual code. RAD’s JSF generator violates quite a few of our coding conventions including:
- By default creates a <div class="codecolorer-container text vibrant overflow-off" style="overflow:auto;white-space:nowrap;">
- Makes all the properties protected instead of private.
- Puts underscores in a lots of variable names.
- Doesn’t camel case things with multiple words.
- Uses wonderfully descriptive defaults for variable names like text1.</ul>
We agreed to make the attempt to clean them up in RAD’s JSF tool, but if it causes problems with say reverse engineering, then we’ll just let it slide. (RAD continues to get my hackles up with its wonderful features.)
I hammered home that the class we were reviewing badly needed tests since it had none. This really shouldn’t have happened, but so far we’re making slow progress on the test driven development front. We agreed the generated class doesn’t really tests for all of it’s generated methods, just the ones which the developers changed or added to.
The style for making comments was fairly open. We all discussed our thoughts on the class and its methods as we scrolled through the code on the overhead. We didn’t use the more formal process of having one person go through all of their comments before moving on to the next person.
The main class that was reviewed was another interesting animal because it had code from both the tech lead and the senior developer in it so it really was a review for both of them. Going through some conditional navigation logic the tech lead had written the comments was made that it needed to be refactored because it was bout 40 lines long anyway, but that much of the code could be moved to the faces-confix.xml file and navigation rules. The tech lead on this project has quite a bit of experience with Struts, but seeing how the senior developer had utilized more of a JSF style for the navigation he realized it was probably a better approach anyways.
We also found some minor things like get methods that didn’t return anything and should probably be renamed:
public void getIrsDeductions()
The other four classes were much faster to review as they were fairly lightweight DAOs and Service classes that called the DAOs.
At the end of the review I went through the comments from Checkstyle. The developers had recently fixed an issue with RAD where it automatically uses tabs for indenting instead of spaces, so the remaining Checkstyle comments were of some use. Despite getting these reports they hadn’t really spent any time looking at them. It turned out 90% of the comments were about problems in the generated code sections and some older code that had been borrowed from another project that used a Java RMI strategy to attach to a proprietary legacy system which again generated a lot of non-compliant code. Still I thing they got some understanding when I explained the cyclomatic complexity warnings and the method size limit which we set to 20 lines for now.
We wrapped up in about an hour with all of the mandatory changes written on the whiteboard and on the developer’s code printouts. We agreed that the fixes will be in within two days and then the tech lead and I will review that the changes were made. There won’t be a formal meeting, possibly just addressed at the daily Scrum in a few days.
I think this first attempt if a little disorganized was generally a success. We managed to share knowledge and catch things a single developer wouldn’t have noticed, even little things like a misspelled variable name,1
. And I don’t think anyone came away feeling negative about criticism of their code. Both of the developer’s were pretty engaged the whole time, and they both asked a lot of questions and had comments to add. So there’s still the followup, and then next week we run another code review.
There seems to be a lot of overlap between XP and Scrum in the Agile world. Somewhere I remember hearing that Bob Martin suggested at Agile 2005 that everyone was moving towards an Agile process that looked pretty much like Scrum with the XP engineering practices. I saw a little background on this on the scrum-development mailing list from Mike Beedle:
Many of us were in a room full of people — a group that included Ron Jeffries, at Snowbird 2001 when Kent Beck said: I stole Scrum and used Scrum in XP (paraphrasing).
This appears to be about where we are with Agile adoption, though we have quite a ways to go on a lot of the XP engineering practices.
After a bit of a restart from the ashes of the Javacast, the Java Posse Podcast has taken up in it’s wake. I like it overall, but it can have a bit of Java boosterism to it. Still they get some pretty great interviews including Joshua Bloch recently. So if you don’t mind a bit of a Java bias it’s a pretty good development podcast. I pick up little things from the hosts every once in a while, things like one of the hosts regularly uses multiple Java IDEs instead of spending the day in a single IDE.