I was re-reading tilly's Random thoughts on programming recently. The first point reminded me of one of my abandoned back-burnered Quixotic quests: convincing my labmates to take up code reviews.
Now, my situation is more difficult than most. I'm not a working private-sector programmer, surrounded by codebases that will be maintained for years or even decades as a matter of policy. I'm a grad student, and much of the code my labmates write is at least putatively of the "proof of concept, write the paper, forget about it" variety. (Never mind that a clever researcher can keep milking the same basic concept -- and codebase -- for several successful papers. It's called a "research stream", and is the pointy end of the spear in the fight against "publish or perish" administrations. But I digress.)
Point is, people don't really think about code maintenance -- and they especially don't think about software engineering principles. (I, of course, am clearly superior to my labmates -- as the (hang on, let me grep) twelve globals polluting my latest project can attest. Ahem.) We're more concerned with learning about our chosen disciplines than learning how to be better programmers.
That's not quite true; I was overstating my problem to make a point. There are a few people here who are, to varying degrees, interested in clean code and efficient hacking. I think the foundation's there, it's just buried under metres of hasty expedience and the wrong kind of laziness. The question is, how can I unearth and build on it?
My first thought is to offer my services as a code reviewer: I'll review your code, and <godfather>some day, and that day may never come, I may ask of you a favour</godfather>. That way, I can learn from their code, and when I suggest alternatives I might be able to kindle some interest in elegance and cleanliness.
Has anyone else tried (and, with luck, succeeded) to introduce code reviews to an initially unreceptive audience?
(Once I get code reviews in, I'll start on pair programming. I wasted hours today on an unrecognized dumbass attack that would have been immediately spotted by a half-conscious fellow-hacker sitting in a chair watching my vim session. Grr.)
|
Re: Advocacy of code reviews: how the heck do you do it?
by kvale (Monsignor) on Oct 17, 2004 at 06:54 UTC
|
I work in an academic department, too, and have also seen the lack of discipline that you mention.
I have had some luck encouraging code review using two methods. First, I ask some folks to look at my code and comment. Often they will do so, and in turn feel more comfortable in asking me to do so. It helps to first put your ego on the line before asking others to do the same.
The second approach is to note that part of the scientific method is reproducibility, so programs should be made publically available along with the papers. Then code review becomes just another part of the peer review process and prevents public shame in distributing buggy, sloppy code.
This has only worked with three people in my dept., but we have a nucleus of code quality that at times really helps my own work.
| [reply] |
|
|
I have had some luck encouraging code review using two methods. First, I ask some folks to look at my code and comment. Often they will do so, and in turn feel more comfortable in asking me to do so. It helps to first put your ego on the line before asking others to do the same.
I should have mentioned that I've tried this approach; the standard response was "I'd love to, but I don't have time". Since the sticking point seems to be time rather than ego, I thought I'd put my time on the line instead.
I like the point about making code available with papers; my supervisor has been talking about this a lot lately (basically, his theory is that more people will cite your paper if they can easily compare their method to yours on the same platform; providing code makes that a lot more likely).
| [reply] |
Re: Advocacy of code reviews: how the heck do you do it?
by dragonchild (Archbishop) on Oct 17, 2004 at 14:19 UTC
|
I have worked in at least 7+ different companies, both large and small. I have had enforceable code reviews of my code in, count them, exactly 1(!) job. I have attempted to institute them in two others, with varying degrees of success. At my current position, what I have been doing is exactly what kvale suggests - I ask my peers to review my code. That serves several purposes:
- I get to verify that my code is good.
- As I'm the strongest Perl developer, they get to see what I consider to be best practices.
- It starts them thinking that code reviews might be ok.
Now, part of the problem I have had in most places is that management has never felt code reviews to be worthwhile. Say all you want about grassroots efforts, and a lot of it is relevant. But, unless some manager (the higher, the better) indicates that code quality is a priority, then code reviews (and testing, for that matter) will never achieve the level of importance that they deserve.
Now, one thing you can use in an academic setting to help prod your peers into code reviews is the threat that bugs will taint their papers. An academic's reputation is only as good as the last paper published. If that paper is disproven because of a coding error ... ouch! Kinda like you don't want to be the guy that drew the design backwards or confused metric with english measurement.
So, once you get them worried about their code quality, you pull out McConnell's books and show them the single best defense against bugs in new code - code reviews. And then you demonstrate how code reviews leads to the single best defense against bugs overall - code reuse, which also helps reduce the time taken writing the new programs. (This last point is especially important for academics because coding is generally the last thing they want to be doing.)
Being right, does not endow the right to be rude; politeness costs nothing.
Being unknowing, is not the same as being stupid.
Expressing a contrary opinion, whether to the individual or the group, is more often a sign of deeper thought than of cantankerous belligerence.
Do not mistake your goals as the only goals; your opinion as the only opinion; your confidence as correctness. Saying you know better is not the same as explaining you know better.
| [reply] |
Re: Advocacy of code reviews: how the heck do you do it?
by stvn (Monsignor) on Oct 17, 2004 at 19:46 UTC
|
There are many excellent points above by both kvale and dragonchild. Here is my two cents for what its worth.
There are a number of ways to go about code review, and I have participated in each kind and found they all have their benefits and drawbacks.
- Group Code Review
This tends to get really boring, really fast (as most meetings here in the corporate world do), especially if it is too detail oriented. However, I have seen a lot of value (given the right mix of people) when more high level elements of the code/implementation are discussed and debated in this format. Now discussing code at too high a level, I would guess, would not work in your world since it would basically be a discussion about your assignment. However, if you stick to how it is implemented, and not what is implemented it may work out. It could also be thought of as a post-assignment exercise if that helps.
- One on One Code Review
This is personally my favorite and have proven to me to be the most fruitful. The basic idea is you sit down with someone and explain your code to them while they ask probing questions about why you did this, and how you did that, etc. A lot of times, your best revelations come from the act of explaining your code too. I have also seen this work with more than one other person as well, although sometimes that degrades into too much side discussion.
- Private Code Reveiws
You give me your code, I go somewhere and review it. This is what you seem to be proposing above. This method can be really valuable, but tends to take a lot of time and many times requires the reviewer to really know their stuff. To be honest, I have only ever known a few people in my career (PerlMonks not included) whom I have felt doing this with was worthwhile.
As I said, I have seen all three of these approaches work, and all three of them fail miserably. The single most important element to success though, is an open mind and a willingness to take and give criticism (preferably constructive) on the part of all involved. Nothing will derail this process sooner, than a reviewee who can't take a little criticism or reviewers who are afraid to speak up.
| [reply] |
|
|
I like your ideas, stvn, but I have a problem where I work: no peers. At least, not in Perl. There is only one other person in the organization who knows any Perl -- she is just beginning, and uses it only for administration tasks. Now, I've reviewed her code, at her request, many times. She has tried to review my code, but simply doesn't know enough Perl yet.
What options do I have for code review?
radiantmatrix
require General::Disclaimer;
| [reply] |
|
|
radiantmatrix
Well, not having peers does present a problem here. If you have enough other coders/programmers/developers in your organization then it might be possible to just talk a little more high level with them instead of code/language specific. Really most C based languages (Java, Perl, etc.) are similar enough that a good programmer (with an open mind) can understand your perl code. I have caused much envy among Java programmer friends showing them elegant solutions using anon. subs and such, which would take a ridiculous amount of code/time to do in Java.
I would find the best <other language> programmer you can find, and try Option 2, since a lot of the benefits of that comes from you having to explain your own code, you will likely benefit even if they don't offer too much thought.
Personally I would really like to see a Code Review section here on Perl Monks, but that might not help you with work-related code.
| [reply] |
|
|
Re: Advocacy of code reviews: how the heck do you do it?
by SpanishInquisition (Pilgrim) on Oct 18, 2004 at 18:22 UTC
|
Enforcement isn't the way. Getting people to WANT them is the way, and that means frequent exchange of ideas. If you work where I work, and people don't want to speak to each other (it's very sad, really) -- they'll just turn hostile and will be a waste of time. You also have to consider if the Powers That Are are actually people you want reviewing your code, or if they are possibly not the same type of programmer as you.
I like closures, for instance -- the java-jocks wouldn't understand them and would call them wrong. Same with dispatch tables, etc. The more academic or twisted you get, the more you get into a position where you know the difference between right and wrong, and sometimes junior programmers or language-centric programmers shouldn't be in the fold deciding what is or isn't good. OO purists are especially bad at this... and it's worse because there are differing styles of OO.
Do you want to make everyone's code the same? That's what you should be asking. It's better to foster culture of wanting to help each other and collaborate, so you get (effectively) real group ownership and such. Then it will be continuous. Code-review sessions will turn into chest thumping in my opinion.
I guess what I'm saying is, when the love of code and design is there, it all just happens. When someone isn't in coding for the love, it won't happen, and they'll just write bad code. Not just globals, not just sphagetti code, but ravioli code as well. Some people just don't care... and I hate being jaded about it, but they just don't care about learning more. Coding isn't rigid, it should flow. And working with people who can't think the way you think (I'm not saying don't think alike -- it's good that they don't think alike), is just going to cause problems. Big ones. It will eventually cause the apathy to spread to you.
Write good code yourself and enjoy it, and maybe your beautiful code will spread to others, maybe not, but maybe. But making people think for themselves ... whoa, that's hard...
Ugh, incoherrant post. I guess what I'm saying is that with the right people, it all just happens -- and the wrong people are always the wrong people.
| [reply] |
|
|
... but ravioli code as well
Ravioli code. I like that :)
Ugh, incoherrant post.
Not at all. I agree with you, when egos get in the room, it can get ugly. I actually went to college for Fine Arts Painting and one of the most important parts of that cirriculum is the critique. I learned very early on that if someone just said "I like you painting, its pretty", nothing got accomplished, and no one learned. But when egos were put aside, and we actually started tearing into some ones work, and being constructive and not just mean, everyone learned something. The same is true for code reviews, the ego MUST be removed from the situation and the whole thing billed as a learning exercise.
| [reply] |
|
|
