In my previous post, I covered the 8 Winning Reasons To Conduct ETL Code Reviews. In this post I cover four different formats for code reviews.
This is the type of code review that most developers think of first. It usually involves the following steps:
- Code review meeting is scheduled
- Developer constructs a code review document - code is frozen
- Code review meeting takes place
- Developer makes changes to the code based on feedback from the code review
- Validation of code changes in response to bugs found in the code review
- Migration to QA/UAT
Studies have shown this process to be very inefficient in terms of time. Why? Because the code review process often involves too many people. Lawrence G. Votta created this graph to show how difficult it can be to schedule a meeting for a large group.
Try to schedule a 2 hour meeting for eight people and it may never happen! The lesson here is to use fewer people for a shorter period of time. This plays into another recommendation of many studies: keep a review session to no more than an hour. The reasoning is that most people can focus and be effective on this type of task for about an hour. Go beyond an hour and your effectiveness decreases. It’s better to break a review into three 1 hour segments rather than one 3 hour marathon session. I’m a big fan of this type of code review. It requires preparation by the developer, and when everyone is in a room together there is a high degree of interaction. In my experience, I eliminate a good number of my own bugs in the code review document construction process. As I assemble the document, it gives me an extra opportunity to validate the code. If I can’t figure out what I was doing, then maybe the code or the in-line documentation isn’t as clear as I thought it was. An advantage of having a code review document is that it can be sent out ahead of the meeting. Reviewers then have the opportunity to see the code before the meeting.
2. OTS – Over the Shoulder
This method involves another developer – presumably someone at or above your expertise level – looking “over your shoulder” as you walk through the code. The lack of documentation could lead to a rambling review, and it doesn’t give the developer the opportunity to reflect on their code before others have access to it. However, I have used this type of review quite effectively for two situations:
- Emergency bug fixes – A bug has been identified in production and it needs to be put in place immediately. There is no time to gather a group and schedule a meeting.
- Formal review follow up – This is used to verify that the bug identified in the formal review has been corrected.
Online tools (such as that from SmartBear Software) allow multiple reviewers to look at the same set of code, at their own pace, and provide feedback that is directly attributed to the section of code with the bug. No scheduled meeting means that the code could be reviewed much faster. There are a number of tools available for collaborative code reviews for development environments, such as Java, due to their line-by-line coding. Unfortunately, no collaborative tools exist for Data Services. Most of these tools are based on code being checked into a version control system such as Subversion. Data Services does use version control, so perhaps they are not far from a collaboration tool for us? I broached the idea over the phone with the SmartBear folks. While there was no commitment on their part, they did point out that SAP is a customer of theirs. So who knows? Code could be reviewed collaboratively by email; however, a graphically oriented development tool like Data Services doesn’t lend itself well to this approach. I would reserve email code review of ETL for very focused areas, or perhaps only for code within a script object. Conference call tools like WebEx or GoToMeeting could be used in the case where the code review participants cannot meet face-to-face. Given how much development is performed remotely by several Mantis clients, this is almost a requirement. These well-known collaboration tools could be used for any of the above code review formats.
4. Pair Programming
This involves teaming two developers together, in a side-by-side environment, where they are both taking an active part in the coding. While two minds are better than one, the cost of dedicating two resources to the same task could be prohibitive. It could also be said that bugs could slip through simply because both developers are too close to the coding.
5. Bonus Coverage: Code Security
No matter which code review method you choose there will always be these questions:
- Is the code you’re reviewing the current code?
- How do you know that you have reviewed all the code?
I don’t have a silver bullet for either of these questions. I think it comes down to the following: Developers have to be trusted and must have a sense of discipline. Could I submit code to production that hasn’t been reviewed? Sure, but that would be unprofessional at the very least, and – most importantly – it could introduce bugs that would reflect badly upon my work quality. One method for reducing the above two issues in a formal code review: List the objects to be migrated in the code review document itself. The version numbers or labels (from the Central Repository) would be listed with the object name. The migration manager would not migrate any objects that did not agree with the list. Next in the this ETL Code Review series: 6 Best Practices for Conducting Code Reviews
References • Cohen, Jason, Eric Brown, Brandon DuRette, and Steven Teleki. Best Kept Secrets of Peer Code Review: [modern Approach ; Practical Advice]. Austin, TX: Smart Bear, 2006. Print. • Votta, Lawrence G. "Does Every Inspection Need a Meeting?" ACM SIGSOFT Software Engineering Notes 18.5 (1993): 107-14. Print. Photo Credit: Pair programming by esti-, on Flickr