The recent issue of InformationWeek features a Q&A session with Ken Thompson, one of the creators of the Unix operating system. (He collaborated with Dennis Ritchie, of C language fame. Since much of SAS is written in C, I daresay there are a few copies of K&R around here.)
One of the questions hit on the topic of code reviews:
Interviewer: Was there any concept of looking at each other's code or doing code reviews?
Thompson: [Shaking head] We were all pretty good coders.
The implication: only bad coders need code reviews.
This is an outdated attitude towards software development. Unfortunately, I've encountered this attitude among some software professionals that I've worked with (outside of SAS as well as within). Why do some programmers (especially those from the "good ol' days") place little value on code reviews?
Perhaps peer code reviews were less important 30 years ago. The constraints around running code were much tighter. There were fewer layers in the stack where bugs could lurk. Memory was scarce, instruction sets were limited. Computer applications were like my 1973 AMC Gremlin: fewer moving parts, so it was easier to see which parts weren't working correctly and predict when they would fail. (Most often, it was the carburetor -- another outmoded concept.)
Also, the discipline of programming was newer back then. Perhaps there was less variability among the approaches you could use to solve a problem with code. If you had the skills to code the solution, maybe your solution would not differ much from that of any other similarly skilled colleague.
Well, those days are gone. Now more than ever, we need code reviews to be a formal part of how we work as SAS professionals (and as software professionals in general). Mark Chu-Carroll spells out many of the reasons in his blog post, Things Everyone Should Do: Code Review. As Mark says, it's not about the problems that a reviewer will catch; it's more about the programmer who prepares to have his/her work reviewed. Knowing that you have to explain this to another person, that someone else will be looking at your work...that can help to keep you on your toes.
In addition to reasons that Mark cites, those of us who work with SAS have a special motivation: we need to pass on the knowledge of a rich legacy code base to our younger workers. And because the SAS language expands with each new release, old-time SAS programmers need exposure to people who can apply new techniques to old problems. Code reviews are one way to help make that happen: improve quality, ensure continuity, and keep it fresh. Who can argue against that?
9 Comments
Amen. There are a number of reasons not to do code reviews:
- time constraints - both total resourcing and latency (Do I really want to tack on more unscheduled time when I am already working nights and weekends? And: If this bug is so pressing and people are waiting on it, does it really need to sit there waiting for two days for suggestions about how we should rename all those variables and filenames?)
- sense of ownership - parts of programming are personal; how you choose names, how you structure methods, etc. pp. Having someone with a radically different opinion chime in on all of your code distances you from professional pride
- reduction in accountability - if it's my code an my code alone that I check in, I am on spot for anything wrong with it. Submitting it to a large amount of group think absolves me from responsibility more than is good for the quality of the codebase
- standardization to the lowest common denominator - holding all of an organizations code to a standard risks replacing excellence with compliance. Rarely is the average you can get everyone to agree on the best possible solution for all problems
My suggestion is to allow anyone to make changes to any code while being completely responsible for the consequences. You can always review any checkin after the fact (that's what source code history is for) and change whatever you don't like. Just be sure that if I end up spending a day merging with your renaming of my variables, it'll show up in my weekly report and my manager might ask your manager questions about the business value of what you do.
If management places a value on code reviews then they will make time for it. If not, then yes, it will add even more stress to a tight schedule.
I'd refer you back to http://scientopia.org/blogs/goodmath/2011/07/06/things-everyone-should-do-code-review/ and the comments that follow which would say if these are your reasons for not doing code reviews, then you are doing them incorrectly.
Yeah what does Ken know, he just helped invented Unix. But maybe this was more of the old programmer-as-craftsman/artist paradigm, where people had pride in their work and took personal responsibility, and weren't expecting to look for new jobs every few years, and didn't need someone looking over their shoulders in order to write beautiful code. Probably wouldn't work in the modern corporate programming shop.
"The implication: only bad coders need code reviews."
'Good programmers' is most likely used in the relative sense as in "I think they are good programmers", which implies that Ken's views of coding is in sync with his co-workers. If everyone is on the same page, using the same style, and the same conventions then a code review is essentially just a way to put a formal process over a discussion about how each problem is solved. If that communication is happening informally (as it sometimes does with programmers who like talking about their work), then a formal process is redundant.
If there are a bunch of programmers with very different views of what they are doing, it in no way implies that they are bad at it. They just have different ways of working and different experiences. In that case a code review would be hugely beneficial because it pushes them to learn from each other and consolidate their views.
Paul.
I agree it isn't so much what your colleagues say as preparing for the review. I can't tell you how many times before I was going to present some code that I found a way to clean it Up, corrected some small error, documented it better.
as one of those "old-timers" I find it disappointing that no one is keen to review code.
I want to encourage these reviews as a way to break me out of ingrained habits that might not appeal to others as they try to unpick at 3am, some defect I might leave behind.
Having been in that situation, I try to write code for maintainability - but old habits won't improve without some review.
Pingback: Traffic report: the most visited posts of 2011 - The SAS Dummy
Pingback: Code Reviews Change Over Time | Today Blog