Code review
Last week Hacker News posted a link to Zeeya Merall's 13 October 2010 article in Nature on Computational science: ...Error; why scientific programming does not compute." It's a very good article. It hits all the highlights, including Greg Wilson's work on Software Carpentry. Greg's writings and other work have influenced my own thoughts on the problems of software in computational chemistry. His essay HPC Considered Harmful helped me understand more about why I prefer to work on user-oriented software rather than specialized HPC solutions. You can listen to an interview with Greg on that topic.
I'm reading through "Making Software", edited by Andy Oram and Greg Wilson. It's a collection of essays on evidence-based research into ways to improve the practice of software development.
One of my favorite methods is code review (specifically discussed here is, traditional post-authoring peer review and not pair-programming style code review). Chapter 18 is "Modern Code Review" by Jason Cohen. It gives some guidelines:
- A code review finds about one defect every 10 minutes
- Code reviews should not last for more than an hour
- A single code review session should not cover more than about 400 LOC/hour
- Code review meetings are best done after individual code reviews
- Individual code reviews have a 15-30% false-positive rate, which are resolved either with access to the author or through group meetings
- 50% of the defects can be found by the author doing a self-code review
Jonathan Lange wrote a short essay titled "Your Code Sucks and I Hate You: The Social Dynamics of Code Reviews" which covers some of the different styles of code reviews. I point out out more because the title reveals some of the difficulties in carrying out code review.
In my own code, well, I'm a lone developer so I mostly do self-reviews. That is, I write the code, put it aside, and come back to it after a day, week or even months, and walk through the code again. My understanding of the problem often improves after having written the code, so I can go back through it and fix my earlier misunderstandings. One new project I'm working on has had about 6 major rewrites as I try out the different ways to put the pieces together.
I'm involved a little bit in a number of free software projects in cheminformatics. I tend to do random inspection reviews, that is, look at the API, documentation, implementation, or run-time performance, and give feedback. In most cases I get the feeling that I'm the first non-core developer who has looked at the code.
I also do code reviews with my clients. A couple of months ago I organized a code review for a project that four of us were involved with. It was the first time that the others - professional software developers with 5-10 years of experience - had done a code review. It found bugs and places where the code could be simultaneously simplified and improved. I was going to say that it found dead code, but it's a better use of reviewer time to use code coverage and tests to find those automatically. It also helped with knowledge transfer of the overall architecture as well as with more appropriate programming idioms in both Python and Javascript.
(Pair-programming is another way to achieve many of the same benefits of code reviews. However, I still believe that going through large chunks of code, on my own, puts me into a different state of focus and contemplation than when I'm working with someone else. In any case, if you aren't doing pair programming or don't want to do it then code review is an easier change to your process.)
I've even done code reviews with students. Some years back I taught an introduction to programming for bioinformatics course. During the last week of the six week course I did one-on-ones with each students and walked through what they had developed. It took a lot of time, but the feedback I got said that the students liked that sort of focused commentary and found it useful.
I would be remiss if I left out that Egon Willighagen is another developer of cheminformatics software who promotes code review. Most of his reviews are done remotely; through email, bug reports, and git.
I think remote reviews are harder because it takes more time to type things out then it does to say them and physically point to them. Code reviews can also be emotionally delicate and draining, which is why Jonathan Lange's essay title includes "... and I Hate You." Email and other non-physical communiations can already come across as rude or harsh, which doesn't help if the participants aren't already part of a good code review culture.
Change in culture
Code review is a technique which study after study shows is a reliable way to help identify software defects and improve institutional learning. If that's the case, why do so many people not use it? As I mentioned before, I worked with professional programmers who had never done code review before.
You might be tempted to say that it's a cultural issue which can be solved with training. This is both correct, and non-helpful information. Just about everything in software is a cultural issue, from choice of programming language on up. What specific reasons keep people from doing code reviews?
People have asked and commented on this question before. Here are some comments culled from [1], [2], and [3].
- "The reason why people don't like code reviews is because they simply don't know how to conduct one."
- "Don't want other people telling you how to do things"
- "People don't like to be criticized"/"Fear of being judged"
- They know their code is ugly and they are embarassed to show it to others"
- "Think it's a waste of time"...
- ... in any case, it takes a lot of time, and isn't as fun as writing code
- Projects don't manage time for review
- Reviews often end up waste time on "formatting, coding style, or naming conventions"
- "Scary to think of your boss counting how many defects you have. Will that come up in a performance review?"
- It's a bunch of busywork, with meetings, tracking comments, etc.
- "There are so many definitions of what a "code review" is. And so many of those definitions result in very bad situations."
- The project just isn't worth it ("Space shuttle software is not what most people need.")
- "Reading code is hard"
- "People are very bad at ... looking at somebody else's code, and focusing on UNDERSTANDING it, instead of CRITICIZING it."
- "It's easier to get lost in other activities that are more immediately rewarding"
- "Usually thought of as outside the scope of a given project"
- "It's an economic decision; code that's 98% correct is "good enough" for our purposes."
- "They are not always effective."
- Code reviews are "a jumbo-sized magnifying glass on problems in your software development methodology." Once everyone looks at all of it then "you realize what you've done has this *amateurish* feeling."
What about in computational chemistry?
I found this comment by LH on March 08, 2007 to be interesting:
AllanL5: Solutions that merely say "Well, be DISCIPLINED about it then!" are ignoring something very fundamental about human nature.It seems a general trend that you think that people in other fields do a better job than in your own. I'm not convinced that chemists are that "disciplined." After all, the quote "A month in the laboratory can often save an hour in the library." is attributed to the chemist Frank Westheimer.
Well, civil engineers are human too, yet they seem to be disciplined enough to look over each other's work before a bridge or skyscraper gets built. Likewise, chemists and biologists are human, yet they seem to be disciplined enough to read other lab members' protocols before adopting new protocols.
Is there anything about code reviews which is makes them especially hard to integrate into a cheminformatics project?
A) Topping the list is that most developers in this field come from science, not software development. Many have little in the way of formal or even informal training. I don't know that many have heard of code reviews, much less experienced the advantages it can bring.
B) Most of the projects I know of are small, with only one or two people working on it. Those with several people often break the code into parts with one person working on each part. (This is Conway's Law taken to its extreme.) How do you find people who are interested in reviewing your code?
Openly publishing your code on the web can help, but managing a public project - getting to the code to work outside of your personal development environment, maintaining the documentation, and so on - is hard. Sometimes it feels like a matter of faith, hoping that someone else will take enough interest in your project to make it worthwhile.
C) Cheminformatics, like I imagine most of science, has a tradition of releasing "complete" research, not ongoing research. The prime means of communication in cheminformatics is peer-reviewed journal articles. Those are supposed to present novel (and hopefully interesting) results. You would think the culture of peer-review would permeate the culture, but it doesn't really. Here's a quote from Helga Kragh's biography on Dirac:
Ehrenfest was much impressed by Dirac but found it difficult to understand the papers of this British wizard of quantum mechanics. He was therefore delighted when on one occasion Dirac was asked a question to which he, just for once, could not give an immediate and precise answer. "Writing very small he [Dirac] made some rapid calculations on the blackboard, shielding his formulae with his body. Ehrenfest got quite excited: 'Children,' he said, 'now we can see how he really does his work.' But no one saw much; Dirac rapidly erased his tentative calculations and proceeded with an elegant exposition in his usual style."I can't say why Dirac was this way. I only point out that his results were the more important matter, and not they way he got to them. On the other side, Erdõs was very open about collaborations and discussing his ideas. I propose that peer review and code review are actually rather distinct ideas.
(Other fields are likely similar to cheminformatics. Perhaps a preprint system like arXiv would change things, but I don't know enough about its impact on physics to say anything about how it affect cheminformatics.)
Concrete steps
I've been thinking about this topic for a while. How do I encourage code review in cheminformatics? There are a few things I've thought of:
Improve your ability to read code
- Get in the habit of reading other people's source code. You don't need to understand the whole thing! If you download a package like OpenBabel or CDK, and use it for something, then look for the code which does that task and try to read it. Does it make sense or does it do something surprising? Does it have the right amount of code and documentation to help you understand what it's doing?
- When you find something you like, let the authors know! That's a big ego boost right there, which makes them want to continue to develop and release their software.
- If you find something that looks wrong, then see if you can a reproducible way to trigger an result that you think is incorrect. Report the problem to the authors, giving the reproducible, what you expected, what it actually returned, and your thought about what looks wrong in the code.
- After you've written a module, or perhaps even while doing it, see if you can find some other project which has done something similar and compare what they did with what you did. Did they do something interesting? Or incredibly strange? If you see something which is really good or bad, then again, let the author know.
Offer to do code reviews
This part is a lot harder. Basically, you're entering a minefield. Best is if you and a close co-worker work together on this. You need to avoid wars over syntax and style, you need to be able to tell each "back off, you're being a jerk!", and you need to separate the code from the person.
Frankly, I don't know how to bootstrap the process. If it starts on an antagonistic or language-lawyer level then it's going to stay that way - and that rarely leads to good code reviews.
These reviews should be max 30 minutes, done with both reviewer and reviewee at a computer. Although I personally prefer doing my initial reviews on paper printouts, annotated by hand, it's much harder to grep dead trees.
Some places have internal seminars, where they talk about ongoing research. Can we do the same with software?Organize code reviews at a meeting
This is harder still. Most of us work so much in isolation (even in a research group or company) and I don't think that's going to change much. My thought is to take advantage of some upcoming meeting and do code reviews there.
I haven't figured out how to do it. At GCC/Goslar there's the "Open Source Software" track, but the ideas I'm talking about are equally relevant to proprietary software. On the other hand, people working on free software don't have to worry as much about revealing internal efforts.
(I also won't be going to Goslar this year. Perhaps the 2012 Sheffield conference?)
Reviews do take time, especially if the reviewers are coming in from scratch. I don't know how well it would work out if we just put an open call out for reviewees. Who really wants to have their first review be in the public like this?
Instead, we could prepare with a review of a couple of components. In a 30-45 minute slot we could review, say, the aromaticity algorithm of CDK (that's probably too esoteric). It would mean a couple of people would review it on their own and have questions prepared, and during the session we present the code review concept, then do the meeting part of the review.
Conclusion
Software development is a fundamental skill in cheminformatics research. I started offering training courses in the field because I believe that researchers can be much more effective at doing science by learning just a bit more about how to program. The Software Carpentry project has similar views, and applies them to the more general area of scientific software development. I (and I'm sure Greg Wilson) also believes that there are organizational ideas from software development which are also applicable to research software development.
I think that code reviews are one of them, and I'm going to try to promote it. Are you with me? If you have any other ideas, let me know!
Andrew Dalke is an independent consultant focusing on software development for computational chemistry and biology. Need contract programming, help, or training? Contact me
Copyright © 2001-2020 Andrew Dalke Scientific AB