in

Ative at Work

Agile software development

Ative at Work

Code Reviews and the Developer Handbook

We’re six months into a project. The code base is 53,000 statements, 2/3 of which are tests.

We have been working 6 months according to a set of standards: test-driven development with unit and integration tests, model-view-control architecture, NHibernate OR-mapping for the persistence layer, and the Castle container for weaving it all together.

Then an external consultant shows up. He has been hired to check the quality of the work to ensure that a “normal” developer can take the code and work with it inside a “reasonable” amount of time.

I convince him to look at a few architecture diagrams, before he starts looking at the code. I try to show him how to find the implementation code corresponding to the tests but he is not interested in the tests. His goal is to ensure that code is properly documented – and in his world this means the implementation only, not the tests.

Instead he starts at the top with one of the client applications, a web-based intranet for editing data in the system. He starts browsing the code – looking through the container and some of the controller and model classes that it uses.

A few weeks later we get a report.

There should be more comments in the code (our guideline is to write simple, readable code rather than spaghetti with comments).

Oh and member variables should be prefixed with an m, value parameters with a v and reference parameters with an r. And the type of a variable must be derivable from its name.

He also raises a number of concerns about the use of NHibernate and Castle. Basically he wants us to write documentation for these as well (never mind they come with documented source and books and on-line articles).

More or less the audit says to use his idea of what constitutes a good project. So basically he is testing the documentation quality against an arbitrary set of requirements.

We need to devote a lot of effort to convince him.

So, note to self – a pragmatic solution for documentation and auditing:

  • Write a simple developers handbook with coding and documentation guidelines etc. and have the customer sign it off. 
  • Involve the auditor early to check the guidelines. 
  • Use the handbook as the guideline for audits. 
  • In any event have the auditor provide a set of “functional requirements” to the documentation, eg. “it should be possible to easily find out what a method is doing” rather than saying arcane things like “there should be a two lines of comments for every branch statement”. 
  • Create a HOWTO document for each of these requirements and add it to the developer handbook: for example, “The requirements are captured in integration tests using the Selenium tool, they are named so and so.” etc.. 

Update Nov. 6, 2006: Please note that I'm not advocating a heavy-weight developer's handbook, just a small document that shows the file structure, naming conventions, how to get the source from version control, how to build and run the tests etc. I've spent time on a project with a big guideline on whether or not to put curly-braces on a separate line etc. and its a waste of time. For code layout I recommend to just write readable code that looks like the rest of the code in the system and rely on the default formatting in IDE.

Published nov 02 2006, 10:15 by Martin Jul
Filed under: , ,

Comments

 

Mark Seemann said:

Ouch, that's a difficult situation to handle, since that external consultant is probably highly trusted by the customer. In any case, there's nothing you can do except what it seems like you ended up doing: Educate the quy.

Regarding code comments, I always like to hit people in the head with Fowler's Refactoring, where you can read (among many other gems) that code comments are usually a code smell.

Regarding the naming conventions, you can always guide people to the official Microsoft naming guidelines (http://msdn.microsoft.com/library/default.asp?url=/library/en-us/cpgenref/html/cpconnamingguidelines.asp). In addition, Code Analysis will issue a warning if you post-fix variable names with type names, so that advise just goes plainly against the official .NET guidelines. We also don't use prefixing anymore.

So, I'm not disagreeing with you at all, but just wanted to point out that for a lot of the issues you mention, you can just point people to the official .NET development guidelines from Microsoft :)

Regarding member variables, I have more to say at http://blogs.msdn.com/ploeh/archive/2006/03/08/CodingStyles.aspx, but that's just my personal style :)

november 7, 2006 9:19
 

Martin Jul said:

In fact, we use the Framework Design Guidelines as a foundation, along with the static code analysis that you mention. FxCop is truly amazing - just make sure to enable it from day one since it will be hard work to fix the issues later. Furthermore it is a good idea take a look at some of the performance rules and turn them off as necessary - while they make for better performing code they are sometimes a bit on collision course with simplicity - an example being that the rule is to prefer returning and enumeration or collection rather than an array.

One of the best articles I've seen about code commenting is by Tim Ottinger of ObjectMentor. His basic point is that a comment is an apology for not writing something that is clear and easy to understand. It is called  Apologies in Code and is is well worth reading - including the debate in the comments: http://butunclebob.com/ArticleS.TimOttinger.ApologizeIncode

november 8, 2006 9:35
 

Jan Daniel Andersen said:

What a perfect timing for this article.

I've been on a project for a little more than 12 months now. With reference to both Ottinger and Fowler, I've been trying to tell my colleagues, that commenting is bad bad bad! They didn't listen. They wanted comments and it was made law, so every public member had to have comments. We even turned on the feature in VS.Net that makes a build fail, if the comments are not in there.

So yesterday I had to access some older part of the codebase. The naming wasn't very good... in fact one could be as bold as to call it cryptic. "But hey" i thought "we have all these nice comments that wil lexplain to me what these cryptic statements mean". But to my (not so great) surprise, there were no comments on this code and the meaning of the code was VERY har to learn.

What would have been an easy task turned into a lengthy "learning" session. Ofcourse the badly name, non-commented code, was also very bad in it's internal structure, so it was very hard to grasp what the original developer had wanted his code to do.

This experience has made me dislike the idea of code comments even more than I used to. It should be obvious to everyone that comments should NEVER take precedence over good naming and structure of the code. It is possible to forget putting in the comments, but you can never forget writing the code; the code HAS to be there, the comments don't, so one should focus on making the mandatory parts as good and readable as possible.

november 8, 2006 12:07
 

Martin Jul said:

Actually with bad code it is often a good example to just go through a refactoring - renaming stuff and extracting methods or introducing Method Objects really go a long way.

With the particular reviewer I mentionend I actually went through this on a piece of code that he particularly fancied - it had a bunch of nested ifs and switches and what have you and loads of comments placed in boxes or * stars. His point was that even though it was complex, it was easy to understand.

After refactoring it to simple code with no comments it became obvious that the code had did not handle some of the border cases correctly (the unit tests did not cover these completely, so the code had multiple problems, not just readability).

To sum it up, the conclusion is all in favour of Tim Ottinger. Code comments ARE apologies.

november 8, 2006 12:38
 

Jan Daniel Andersen said:

Yeah ... I totally agree. Both with Ottinger and you. The problem is that we're using a single checkout Source Control System that is unable to merge code. I could refactor the bad code, but that would have to be done after hours when no one else was working on the project. It can get extremely complicated to refactor, if you don't have access to all the code (because it is checked out by others), that needs to be changed.

Usually I just change the bad code I encounter right away, as long as the changes isn't too extensive, but sometimes the changes cascades out to be a huge change, that is not readily done. You can really come a long way with "extract method" and some creative renaming!

november 8, 2006 12:51
 

Martin Jul said:

Jan, have a look at the excellent presentation "The Prime Directive. Don't Be Blocked" (http://butunclebob.com/files/SDWest2006/ThePrimeDirective.ppt) by Uncle Bob. He argues that in the case of a blocking version control system like that, use a non-blocking version control system (eg. Subversion) inside each sprint and only do a check-in to the other "official" source control system periodically. This way, you're not blocked but you're still compliant with corporate policy (sort of...).

november 8, 2006 2:00
 

Mark Seemann said:

Comments (and documentation in general) are not bad, and I want to oppose the statement that they are. IIRC, what Fowler means (and what I personally mean) is that *inline* comments are an excuse for proper naming. That doesn't mean that you shouldn't fill out meaningful XML commentf for your public API, but that concerns the hard part of writing out all the stuff that can't be automated (by tools such as GhostDoc). Be honest now: Can you really get by with just the naming of types and members in the .NET framework? If you are like me, that's probably enough more than 50% of the time, but still, there are times when you'll have to resort to the documentation. What do you look for in the documentation? I know I look for a brief overview that tells me the purpose of the code, and then I look for an example. It's not always that a proper name can completely capture the purpose of the code, so documentation (i.e. XML comments) still has its place.

Regarding code analysis: If you encounter a rule where you disagree, you can obviously turn it off, but before doing that, be sure to read the documentation for that rule carefully to ensure that you fully understand the reasoning behind the rule. I find the vast majority of the rules to be perfectly reasonably, once you understand their motivation. If you are still convinced that the rule is counter-productive, I'll strongly suggest that you provide this feedback to the FxCop team (e.g. via http://blogs.msdn.com/fxcop/). I've done this quite a few times, and have always found the team very responsive to feedback, and I know for a fact that if you have your arguments in place, you can convince them to change or even drop a rule - just be prepared that you may have to convince people like Krzysztof Cwalina :)

Concerning non-blocking source control, Team Foundation Server has that :)

november 9, 2006 12:04
 

Martin Jul said:

Mark, that's an excellent point. The .NET Framework documentation is definitely useful. So, I have to give you that.

On the other hand - not a lot of people develop frameworks. Many projects think they do, but most of the time they are really just building an application: all the developers using it will have access to its source and its unit tests, so they will have plenty of documentation available at no extra cost to the project.

My experience with code comments is that when they are mandatory you will have something comments like "Get the First Name" on a property called FirstName. So, most of the the comments add no value anyway.

We once worked on a big RUP project (jokingly called document-driven-documentation since we had more than 10000 pages of design documents with nicely coloured UML-diagrams). When we did the integration we hardly ever touched the docs. Number one tools was looking at the assemblies in "Lutz" (Reflector - http://www.aisto.com/roeder/dotnet/), next to look at the source and sometimes even the unit test, but hardly ever the external documentation. It would be out of synch, incorrect and since it was written upfront when nobody knew what was needed it had pages of the simple stuff like API and interface descriptions which were availble in the code anyway, and class diagrams (also available in Lutz) and other stuff but never the stuff we needed to know. So the net value of the huge investment that was made documentation the system was close to zero. And it even pretended to have a "framework" at its base.

november 9, 2006 9:07
 

Martin Gildenpfennig said:

I agree that third party frameworks should come with documentation. But in application development I have yet to experience that XML comments are worth the time spent writing them. As Martin Jul puts it, people have access to all code and it has all the information they need. If it’s properly structured its even easily readable, so why bother with the extra work of translating the code to prose?

On my current project I believe most methods have XML comments. Still, I mostly see programmers trying to understand the code using Lutz Roeders Reflector (the application has quite a few assemblies, that’s why this is easier than using VS.NET). Nobody uses the XML comments and the reason is that the XML comments are either incorrect or GhostDoc-style. I think this is the common case on most projects.

Regarding inline comments I completely agree with Tim Ottinger: they are apologies. The best example I can think of is from my current project where an inline comment reads: "The following code is error prone. I made it myself". The programmer even wrote his initials next to the comment :)

november 9, 2006 11:14
 

Søren Lindstrøm said:

I can agree with you for the most part - however code comments are usefull, if done correctly. There are three types of comments:

- "How does the det code do what it does" (BAD: Sorry but I only write encrypted code).

- "What does the code do" (BAD: Sorry but the comment must be updated everytime this method is changed).

- "What do I intent the code to do" (GOOD!)

The last type of of comment carry information that can never be captured in written code: A high level abstraction of what the original developper wanted this method to do.

Yes this information can to some degree be captured in Unit Tests, but face it, you would probably have to capture this information from several tests, AND it will only provide this information IF the unit tests are made TDD.

In fact most unit tests would benefit profoundly if the author would write a short comment on what he INTEND to test.

However it is hard to write this type of comments as Steve Mcconnell has noted.

november 10, 2006 12:33
 

Mark Seemann said:

Well, what all the Martins (including Fowler, I'd guess) seem to be saying is that they quite prefer meaningful names instead of comments, and I can only agree. GhostDoc-style comments are worthless, but what I was trying to say is that sometimes, you do simple things for complex reasons (like the one you describe at http://community.ative.dk/blogs/ative/archive/2006/09/29/Migration-_2D00_-Bug_2D00_by_2D00_Bug-Compatibility.aspx), and a meaningful name may just be too cumbersome. In this case, a code comment explaining what the code does, and why it's there, may save other developers some time down the road.

november 13, 2006 10:20
 

Jan Daniel Andersen said:

Marin J: About being blocked... what do you do, when blocked by old-fashioned politics ? I cannot use another SourceControl system. If I do i get fired. But that would probably be a good thing :-). I do agree about "not being blocked"; I'm a frequent reader of Bob's blog.

With regards to Xml Comments, my point is that it shouldn't be mandatory. If you cannot make the purpose clear through good naming and wellstructured code, you SHOULD use a comment. However, in my experience, those cases are pretty rare. I often refactor even simple methods, to make their purpose even more clear, so you can cast a quick glance at it and get the idea right away.

It is possible to refactor your code to a point where it is almost as readable as plain text; although that would probably be a bit over the top. I just like to rely on looking at the actual code instead of hoping that my team mates remembered to update their comments. And ofcourse there is the .Net Reflector; IMO one of the best developer tools ever made!

november 13, 2006 12:40
 

Søren Lindstrøm said:

About being blocked and company politics: Without knowing if Martin Jul is refering to Uncle Bob when taking about being blocked, I think being blocked is a reminder to us as developpers/architects as much as companies.

If you are being blocked because of old fashion politics mandate that you should use a defective source controle, your company have a problem (and believe me - they know it if this is the case).

If you are being blocked because you must use a source control system that isn't state og the art, you have a problem of attitude.

If you are being blocked because your company isn't state of the art on all tools (e.g. by using VSS instead of SubVersion), you would be blocked in 95% of all companies!

Of cause you should let your company know that there are better products available, but if you can't work on anything but the best of the best, you may have earned yourself a spot on the Novell NetWare Code-guru team.

november 14, 2006 9:44
 

Jan Daniel Andersen said:

Hehe ... this discussion wasn't really about Source Control Systems. It was about the way we work, and how that can sometimes be a blocking mechanism. If I cannot access all the code that should be changed due to a refactoring, it's very hard to do that particular refactoring.

This means that in the case I describe, I cannot refactor the badly structured pieces of code to a point where no comments are needed... I guess comments ARE a good thing on this projekt. So one could conclude that comments are good if you follow a really bad work process.

I still like to rely on well structured code with meaningfull naming... as an old colleague used to say: "Naming is everything".

november 15, 2006 12:22
 

Frederic Gos said:

Jan! Stop whinning and go back to work!

You probably have a form to fill and send for the next database change. And after that you have a build to perform. That shouldnt take more than 2-3 days!

december 29, 2006 1:38
 

Tim Ottinger said:

This is Tim Ottinger from Object Mentor (quoted above, thanks Martin Jul!).  

I think that it was kind (and appropriate) of you not to mention the company that consultant came from. We actively recommend not using warts or encodings, writing simple code with meaningful names (including for explanatory variables and functions), and using tests to explain code.  

I think that the industry is turning this way as a general rule, with large and stodgy companies typically maintaining the old status quo (test-last, individuals rather than pairs, comments rather than clarity).  Things will turn your way eventually, or sooner if we can make it so. It may take a little while.

januar 23, 2007 10:39
 

Andy Dent said:

I think there are a few places where comments are extremely useful, if not essential.

When writing frameworks, comments make it easy to document the product - I'm a big fan of Doxygen, see for example http://www.oofile.com.au/oofile_ref/html/ which is generated from our code.

In particular, products like Doxgyen let you use comments to tag areas of code as being part of a particular group, allowing you to describe associations between code that your programming language may lack.

The most essential use of comments, which I don't think anyone has mentioned, is to document the weird behaviour of system APIs, other people's code or typical traps. Yes these are apologies but they are apologies for someone else's behaviour, like a Pedestrians Beware sign in front of a hole in the sidewalk.

For example, an inline comment might read "you may be tempted to use the GetDocument() call here but although it compiles it returns an empty document, hence our more complex use of GetHtmlDocument()".

The bottom line is - will this comment Save or Waste someone else's time?

januar 24, 2007 10:36
 

Andy Dent said:

Following the links to Ward's Wiki, I found a great example: http://c2.com/cgi/wiki?BlindAlley

januar 24, 2007 10:43
 

Andy Dent said:

A few more thoughts (literally stuff I woke up with) prompted me to come back and re-read this posting.

On the topic of "external consutants" - given his very specific mandate, it might be worth challenging management as to the relative cost-benefit. Say you assigned a couple of simple programming tasks, that required understanding the code base, and they just went out and hired someone to do them. Would this have been cheaper than the consultant? It would certainly be a more scientific test of the "pickupability" of your source base.

I really like the list of things for your programmer handbook however you missed something, or maybe didn't bother making it explicit. You say that the libraries are well-documented. Does your handbook or code have links to that documentation? Do you also have locally cached copies that are going to be permanently retrievable?

What happens in a year's time when someone is doing maintenance and wants to use the version of those external libraries you were using at the time? Have you made sure all those dependencies are archived with a release?

good luck

Andy

januar 26, 2007 3:32
 

Martin Jul said:

Andy

Thanks for your comments.

As a rule of thumb I think everything that is needed to build the system should be in placed under source control so I can set up a new machine by just fetching the project from source control and get going. So as you might have guessed we did indeed put the external libraries (NHibernate, Castle Project, Rhino Mocks, Selenium etc) in the source control as well. I usually keep both the source package with docs and everything and the binaries that our application needs to reference. This way it is easy to upgrade to later versions and everything will be there in the future.

I know some people don't like the idea of putting "too much" (binaries etc) under source control but hey - for me, disk space is so much cheaper than not being able to build the system.

In fact, I even know of some people who put all their tools including compilers unders source control (embedded systems guys) since they sometimes have to fix the software on a machine that has been sitting in a field for ten years.

januar 29, 2007 10:27
 

Martin Jul said:

Tim Ottinger of Object Mentor wrote an article about this post at

http://blog.objectmentor.com/articles/2007/01/23/good-things-come-eventually

Here is some more background information on the case mentioned that I have posted as a comment to Tim's article on his blog:

Let me provide some more background on the case.

One that particular project I was a consultant (architect/tech lead) working with a customer’s team. Our customer itself was another consulting company working for an enterprise organisation. The project was to replace their legacy mainframe with a .NET application. The enterprise organisation had hired a code reviewer to check the project. He came from yet another consulting company.

We used standard, but modern techniques like TDD, an inversion-of-control container (Castle Project) and an OR-mapper (NHibernate).

The consulting company we helped learned to appreciate our agile practices – initially there was the normal resistance to change but in the end it is hard to argue against results.

For example, when we introduced the OR-mapper (NHibernate) the DBAs put up a fight since myth has it that Stored Procedures always perform better. Only when we measured and showed that it did in fact perform well and that it would free up four DBAs from writing SPs for many months did the resistance die out. It is hard to argue against facts like that.

The resistance from the code reviewer was basically due to his assignment. He was asked to review the documentation. The intention was to check the documentation to make sure that the enterprise organisation could hire someone else to work on the application afterwards. The underlying agenda was to avoid supplier lock-in. This, of course, is perfectly reasonable.

The problem was that the reviewer had almost fourty years of experience in application development but none with unit testing, mocks, inversion-of-control, OR-mappers and Selenium (automated web integration testing).

From that perspective, documentation is the green comments in the source code – the XML that can be extracted to a help file. And – since comments provide the only value from this perspective – no value was assigned to properly named methods or simple, readable code with tests that document the expected behaviour.

He actually managed to find one piece of code in the system that he liked. It had comments (apoligies!) – it was a complicated business rule reverse engineered from the legacy system.

To make a point I refactored it to its simplest extreme and removed all the comments and at that point it became apparent that the business rule was specified in a way that made no sense.

From my perspective that proved the case for readable code.

However from the perspective of the organisation paying for the project all they saw was consultants disagreeing about the right approach.

Eventually however some Asian consultants were in-sourced to the project to write a bunch of data export jobs and after we proved that we were able to get them up to speed quickly we did not have any more interference from the code reviewer.

So the story ends well.

There will be resistance to new ways of working, but it is possible to overcome it by showing good results.

The big lesson learned was that if code reviews or other external factors are allowed to interfere there is good wisdom to be found in the principles behind the Agile Manifesto – “customer collaboration over contract negotiation”. Focus the work on proving that the method is sound by showing good results.

For non-developers direct experience is much more powerful than quarrelling over abstract concepts of “good code”, so demonstrating that their concern about vendor lock-in was addressed properly removed the obstacles.

That’s another facet of agile development.

januar 29, 2007 10:31
 

Anu said:

Can you please let me know how u convience ur DBA's (refer a text part taken from your blog)

"For example, when we introduced the OR-mapper (NHibernate) the DBAs put up a fight since myth has it that Stored Procedures always perform better. Only when we measured and showed that it did in fact perform well and that it would free up four DBAs from writing SPs for many months did the resistance die out. It is hard to argue against facts like that."

I need some input for the same please email me ilovtobeloved@gmail.com

thanks in advance

Anu

maj 16, 2007 7:45
 

Martin Jul said:

Anu -

to prove that NHibernate was "good enough" we set up an experiment.

We used the mainframe database and wrote an application with two different persistence layer implementations. One was written by the application team with NHibernate, the other was written by the DBAs and used stored procedures.

When we measured the implementations against each other we were quite surprised. NHibernate won in two respects:

First of all the DBAs much more time writing the persistence layer than we spent on the NHibernate version (if I remember correctly, about 4 times more).

Then it turned out that NHibernate queries were faster than than the code written by the DBAs - it used fewer reads for its queries. The DBAs were quite dumbfounded by this. When we looked at the NHibernate code in the query analyser they concluded that "they could have written that, too". They were very skilled, so they had the ability, but from an economical point of view it made much more sense to go with NHibernate since the cost in terms of manpower was much less.

And from an agile point of view I would always start by doing that and then only resort to stored procedures or whatever other tricks when we measure that we have actually hit the limits of the OR-mapper. This way, you can probably get 90%+ of the persistence layer at a very low cost and maybe implement a few critical operations as SPs if necessary, rather than taking the huge cost of implementing it all by hand.

The key is that using an OR-mapper does not remove the option to add some handwritten database code later if necessary.

One word of caution, however. When you design you domain model think carefully about the "aggregate" pattern from Domain-Driven Design (http://domaindrivendesign.org/) - it is very important to design you model in small, independent parts so you don't have to pull the whole database into memory to work on it.

maj 20, 2007 11:20

About Martin Jul

Building better software faster is Martin's mission. He is a partner in Ative, and helps development teams implement lean/agile software development and improve their craftmanship teaching hands-on development practises such as iterative design and test-first. He is known to be a hardliner on quality and likes to get things done-done.
© Ative Consulting ApS