rc3.org

Strong opinions, weakly held

Four reasons to file the bugs you find in code review

I have a habit, some would call it a bad habit, of fixing bugs whenever I see them, usually without ever filing a case in the bug tracking system or telling anyone that I found the bug at all. Usually this happens when I’m reading through code I’m not ostensibly working on. When I see bugs, I fix them. Someone with fewer battle scars than I have may think that’s a great habit and that there’s no need to apologize. Here’s why I should always file a case, even if I fix the bug right then:

  1. Some other developer may unknowingly (or worse, knowingly) depend on behavior that results from the bug. In other words, fixing the bug may break something else.
  2. Your customer may expect or worse depend on the behavior caused by the bug. Bugs that are found in code review are usually issues with business logic. If the bug causes the app to blow up, you rarely need a code review to find it. Customers deal with the interface they’re given, so business logic or formatting issues that look like bugs internally can become part of the de facto contract you have with them. Making changes without telling anyone can create unpleasant surprises.
  3. In fixing the bug, you may break something else. Any time you edit code, you may introduce a defect. Heck, some developers are as likely to introduce a defect as not. If you change code without telling anyone, you may fix one bug but introduce one that’s even worse, or you may not really fix the bug.
  4. Your testers need to know what to test. Your changes may not get tested if you don’t tell anyone what was broken or why and how you fixed it.

Update: Be sure to check out frequent commenter Stan Taylor’s follow up post on this topic.

6 Comments

  1. While not as important as most of what’s on your list, filing that bug also provides visibility to management/clients about what you as a developer do/are doing. Never hurts to have that, but this is especially the case if your managers are non-technical doofuses (Not that this ever happens) or you are billing a client hourly.

  2. To expand on the last point, “every bug deserves a test”; that is, it is usually the case that a bug reflects a type of failure that is not obvious and could easily be repeated, otherwise it wouldn’t be a bug. So put a test in to catch it (or if it’s a UI bug, put a test in the test plan for it).

    I’m not very good at this kind of thing either though.

  3. These are all about communication, and the assumption that filing a bug is a good way to communicate with developers, testers, and customers (and as zack adds, managers). I guess it depends on the local culture, but in my experience, developers only look at a bug report if it’s assigned to them. The revision control system is a better way to see what’s happened recently. There will always be a check-in when a bug is fixed, but other check-ins are relevant to everyone you mentioned and could just as easily cause any of the same problems. Will you also file a bug report when you check in new code that doesn’t fix a bug?

  4. Generally all the code we check in is associated with an item in the issue tracker, either a feature request or a bug.

  5. Great post! These are very good reasons for submitting bugs during code review. Too often the issues found during code review get lost in the shuffle.

    Atlassian has tools for both bug tracking and code review, and we’ve integrated them such that you can create bugs from code reviews with just a few clicks: http://www.atlassian.com/software/crucible/tour/jira-integration.jsp

  6. @Seth – As a QA engineer, my nirvana is a system that shows work item IDs (defect, requirement) completed with each commit (or build) as well as a list of files changed with diffs (Let’s not get bogged down here in the discussion of whether EVERY commit must have a work item). That provides both angles: the documentation (defect) angle and the ability to see what exactly changed.

    Having the ability to look at diffs (preferably with an indication of why something changed, hence associating work item IDs with commits) serves several purposes: 1.) it allows me an easy way to familiarize myself with the code, 2.) it helps me decide how I need to test something, and 3.) it allows me to spot unreported changes and, at the very least, ask the developer why he changed it.

Leave a Reply

Your email address will not be published.

*

© 2024 rc3.org

Theme by Anders NorenUp ↑