{"id":835,"date":"2009-10-08T13:29:19","date_gmt":"2009-10-08T18:29:19","guid":{"rendered":"http:\/\/mikeconley.ca\/blog\/?p=835"},"modified":"2023-12-20T16:25:18","modified_gmt":"2023-12-20T21:25:18","slug":"the-achilles-heel-of-light-weight-code-review","status":"publish","type":"post","link":"https:\/\/mikeconley.ca\/blog\/2009\/10\/08\/the-achilles-heel-of-light-weight-code-review\/","title":{"rendered":"The Achilles’ Heel of Light-Weight Code Review"},"content":{"rendered":"

So I had my weekly meeting with my supervisor<\/a>, and fellow students Zuzel<\/a> and Jon Pipitone<\/a>.\u00a0 Something interesting popped up, and I thought I’d share it.<\/p>\n

If it wasn’t already clear, I dig code review<\/a>.\u00a0 I think it really helps get a team of developers more in tune with what’s going on in their repository, and is an easy way to weed out mistakes and bad design in small chunks of code.<\/p>\n

But there’s a fly in the soup.<\/p>\n

This semester, the MarkUs project<\/a> has been using ReviewBoard<\/a> to review all <\/em>commits to the repository.\u00a0 We’ve caught quite a few things, and we’ve been learning a lot.<\/p>\n

Now for that fly:<\/p>\n

A developer recently found a typo in our code base.\u00a0 An I18n variable was misspelled in one of our Views as I18n.t(:no_students_wihtou_a_group_message).\u00a0 One of our developers saw this, fixed the typo, and submitted the diff for review.<\/p>\n

I was one of the reviewers.\u00a0 And I gave it the green light.\u00a0 It made sense – I18n.t(:no_students_wihtou_a_group_message) is clearly wrong, and I18n.t(:no_students_without_a_group_message) is clearly what was meant here.<\/p>\n

So the review got the “Ship It”, and the code was committed.\u00a0 What we didn’t catch, however, was that the locale string was actually named “no_student_without_a_group_message” in the translation file, not “no_students_without_a_group_message”.\u00a0 So the fix didn’t work.<\/em><\/p>\n

This is important:\u00a0 the diff looked good, but the bug remained because we didn’t have more information on the context of the bug.\u00a0 We had no information about I18n.t(:no_students_without_a_group_message) besides the fact that I18n.t(:no_students_wihtou_a_group_message) looked wrong.<\/p>\n

Which brings me back to the conversation we had yesterday:\u00a0 while it seems plausible that code review helps catch defects in small code blocks, does the global defect count on the application actually decrease?\u00a0 Since ReviewBoard doesn’t have any static analysis tools to check<\/em> what our diffs are doing, isn’t it plausible that while our diffs look good, we’re not preventing ourselves from adding new bugs into the code base?<\/p>\n

So, the question is:\u00a0 does light-weight code review actually decrease the defect count across an application as a whole<\/em>?<\/p>\n

If not, can we augment these code review tools so that they’re more sensitive to the context <\/em>of the diffs that are being reviewed?<\/p>\n","protected":false},"excerpt":{"rendered":"

So I had my weekly meeting with my supervisor, and fellow students Zuzel and Jon Pipitone.\u00a0 Something interesting popped up, and I thought I’d share it. If it wasn’t already clear, I dig code review.\u00a0 I think it really helps get a team of developers more in tune with what’s going on in their repository, […]<\/p>\n","protected":false},"author":4,"featured_media":0,"comment_status":"open","ping_status":"open","sticky":false,"template":"","format":"standard","meta":{"jetpack_post_was_ever_published":false,"_jetpack_newsletter_access":"","_jetpack_dont_email_post_to_subs":false,"_jetpack_newsletter_tier_id":0,"_jetpack_memberships_contains_paywalled_content":false,"_jetpack_memberships_contains_paid_content":false,"footnotes":"","jetpack_publicize_message":"","jetpack_publicize_feature_enabled":true,"jetpack_social_post_already_shared":false,"jetpack_social_options":{"image_generator_settings":{"template":"highway","default_image_id":0,"font":"","enabled":false},"version":2}},"categories":[454],"tags":[501,509,508,491,494,504,510],"class_list":["post-835","post","type-post","status-publish","format-standard","hentry","category-code-reviews","tag-code-review","tag-context","tag-light-weight","tag-pcr","tag-peer-review","tag-reviewboard","tag-static-analysis"],"jetpack_publicize_connections":[],"jetpack_featured_media_url":"","jetpack_shortlink":"https:\/\/wp.me\/prmTy-dt","jetpack_sharing_enabled":true,"_links":{"self":[{"href":"https:\/\/mikeconley.ca\/blog\/wp-json\/wp\/v2\/posts\/835","targetHints":{"allow":["GET"]}}],"collection":[{"href":"https:\/\/mikeconley.ca\/blog\/wp-json\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/mikeconley.ca\/blog\/wp-json\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/mikeconley.ca\/blog\/wp-json\/wp\/v2\/users\/4"}],"replies":[{"embeddable":true,"href":"https:\/\/mikeconley.ca\/blog\/wp-json\/wp\/v2\/comments?post=835"}],"version-history":[{"count":7,"href":"https:\/\/mikeconley.ca\/blog\/wp-json\/wp\/v2\/posts\/835\/revisions"}],"predecessor-version":[{"id":3216,"href":"https:\/\/mikeconley.ca\/blog\/wp-json\/wp\/v2\/posts\/835\/revisions\/3216"}],"wp:attachment":[{"href":"https:\/\/mikeconley.ca\/blog\/wp-json\/wp\/v2\/media?parent=835"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/mikeconley.ca\/blog\/wp-json\/wp\/v2\/categories?post=835"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/mikeconley.ca\/blog\/wp-json\/wp\/v2\/tags?post=835"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}