Code Reviewing - Improving our process

We have implemented a code review process over the past 3 years which seems to work reasonably well. This helps considerably to reduce the number of defects QA and ultimately the customer will see. However it’s only recently that I’ve started to analyse our current process with a view to improve it.

One of my biggest concerns is that we find a lot of non functional defects and not many functional defects in code review. Whilst the non functional defects are important in terms of helping with code maintainability, functional defects should be the top priority on any code review as these defects usually manifest themselves as issues which would be visible to a customer.

I have recently read a copy of “Best Kept secrets of peer code review” which is freely available from smart bear software at http://smartbearsoftware.com. It provides useful techniques which can be used to improve your code review process and are based on real life case studies. Although of course the book does lean towards using smart bears code collaborator product to help with the code review process it does have a lot of useful content to warrant reading it even if you’re not likely to purchase these tools.

Some points I picked up from the book which I’m going to look at in improving our process are

1. Initial quick scan – The first initial scan can be very important in helping find defects throughout the code. As there is a negative correlation in the time the reviewers takes on the first scan versus the detected speed. The more time the reviewer takes on their first scan, the faster the reviewer will find defects. If you don’t perform a preliminary scan your first issues identified may focus on the wrong areas. With a quick scan you can identify areas with the highest possible issues and then address each one with a good change you’ve selected the right code to study.
2. Review for at most one hour at a time – The importance of reviewing in short time periods shouldn’t be underestimated. We should try and avoid large reviews and try and split them up into smaller reviews where possible. E.g. you could certainly split up reviews of unit test codes versus core code. Although of course this isn’t an excuse to submit code with known defects for review.
3. Omissions are hardest defects to find – It’s very easy to find issues for code you’re reviewing, it’s much harder to find defects in code which isn’t there. Having a review check list illeviates this issue some what to remind the reviewer to look for code which may not be present. Defects that aren’t associated with any checklist item should be scanned periodically. Usually there are categorical trends in your defects; turn each type of defect into a checklist item that would cause the reviewer to find it.
4. Defect density and author preparation – Providing some preparatory comments before code is submitted for review does seem to have an impact on defect density. The key point here is that by annotating certain areas of code, developers are forced to do some kind of self review of their code which obviously increases the quality. Although there is a danger that the developer by preparing could disable the reviewer’s ability for criticism i.e. he’s not thinking afresh. However usually by developers preparing these kind of comments makes them think through their logic and hence write better code.
5. Leverage the feedback you’re getting - Improve your ability and make yourself a better developer by reviewing the points you commonly receive. Code review points should be learned so you reduce the number of defects over time. A useful exercise could be to put your comments in your own personal check list which you visit when ever performing a code review. Any points which aren’t covered on the global check list should be added
6. Developer and reviewer joint responsibility – Defects found in code review are good. A defect found in review is a defect which never gets to QA or the customer. Developer and reviewer are a team and together plan to produce excellent code in all respects. The back and forth of coding and finding defects is not one developer punishing another, it’s simply a process in which two people can develop software of far greater quality than either could do single-handedly. reviewer finds in the developers code the better, this should be thought excellent summary
7. Checklist – Use check lists as an aid to identify common defects. The most effective way to build and maintain the checklist is to match defects found during review to the associated checklist item. Items that turn up many defects should be kept. Defects that aren’t associated with any checklist item should be scanned periodically. Usually there are categorical trends in your defects; turn each type of defect into a checklist item that would cause the developer to find them next time.

Design - Transitioning from conceptual domain to physical data model

When creating the design for new requirements we often design a domain model of key entities and then when were happy with it we migrate this domain model into a logical and then physical data model. Although we should go to the logical model and then transition to the physical data model we often go straight to a physical data model.

We currently perform this process manually however Enterprise Architect ( UML modelling tool ) has got great support for transforming a domain model into a data model using model driven architecture ( MDA ) templates. It simply reads the entities and the multiplicity relationships between them and generates an appropriate data model. Handling many to many relationships isn't handled with the built in templates however you can easily customise the transformation template using an editor e.g. you can specify

%if attTag:"columnLength" != ""%
length=%qt%%attTag:"columnLength"%%qt%
%elseIf classTag:"columnLength" != ""%
length=%qt%%classTag:"columnLength"%%qt%
%endIf%

to use tagged values to denote the length of a character string in the domain model. Although you shouldn't be too concerned with lengths of strings in the domain model, if you want to specify this information in a tagged value you can at this point, and then ensure it's carried over to the data model e.g. to create a varchar(100) field for a UserName attribute of a User entity. Sparx support are currently developing these templates further and the latest version of the template handles many to many relationships by creating a link table in the format JoinTo however you could customise this to fit your own database standards.

It would be great if through automation we could eventually create a data access layer from the conceptual model using the following steps

1. Create the conceptual domain model. A business analyst would ideally create the first draft
2. Add attributes as required to turn this model into a logical data model
3. Transform the logical data model into a physical data model

We actually use LLBLGen ( object relational mapping tool ) to generate a data access / business object layer from the database so in this way we could continue this process and generate the LLBLGen classes by following some additional steps

1. Create the database from the physical data model.
2. Create the LLBLGen classes from the database directly.

Both of these steps can actually be automated through the automation ability within Enterprise Architect and LLBLGen can be automated too to forward engineer the generated code. In this way we could go from a logical model to a generated database and then to an auto generated business object / data access layer automatically.