Psychic debugging: When reading unfamiliar code, assume it’s mostly correct


You may be called in to study a problem in code you’ve never seen before or be asked to look over a proposed change to some code you’ve never seen before. When this happens, you have to take shortcuts in your analysis because following every function call to the bottom would not only take far too much time, but also take you so far away from the code in question that you will probably forget what you were looking for in the first place.

For example, suppose you’re looking at some code that goes like this:

...
Gizmo *gizmo = get_gizmo_from_name(name);
if (gizmo) {
 Gizmo *parent = gizmo->get_parent();
 parent->set_height(newheight);
 ...
}

You might have some questions about this code.

  • What if name is NULL? Is it legal to pass NULL to get_gizmo_from_name?

  • What if the gizmo doesn’t have a parent? Is there a potential NULL pointer dereference here?

  • Are the gizmo and parent reference-counted? Did we need to do something like gizmo->Release() or a parent->Release() to keep the reference counts in balance and avoid a memory leak?

Finding the answers to these questions may take some time. For example, you might have access only to the diff and not to the entire project, or a grep for the definition of get_gizmo_from_name in the same directory that has the function in question doesn’t turn up anything and you have to expand your search wider and wider in an attempt to find it.

This is when you invoke the “Assume it’s mostly correct” heuristic. The theory behind this heuristic is that whoever wrote this code has a better understanding of how it works than you do. (This is a pretty safe bet since your knowledge of this code is approximately zero.) The problem you’re looking for is probably some small detail, an edge case, a peculiar combination of circumstances. You can assume that the common case is pretty solid; if the common case were also broken, the problem would be so obvious that they wouldn’t need to ask an outsider for help.

Therefore, look at the other parts of the code. For example, you might find a code fragment nearby like this one:

 // rename the gizmo
 Gizmo *gizmo = get_gizmo_from_name(oldname);
 if (gizmo) {
  gizmo->set_name(newname);
 }

That already answers two of your questions. First, you don’t have to worry about checking the name against NULL because this code fragment doesn’t check, and by the heuristic, the code is mostly correct. Therefore, NULL is most likely an acceptable parameter for the get_gizmo_from_name function. Because if it weren’t, then that code would be broken too! (This is sort of the counterexample to what Mom always told you: If everybody else jumped off a bridge, then it is probably okay to jump off bridges.)

Second, this code doesn’t do anything special when it’s done with the gizmo so it’s probably okay just to abandon the gizmo without need to do any special reference count management. Because if you had to dispose of it in a special way, then that code would be broken too!

Now, of course, this heuristic can be fooled, but if you’re operating with only partial information, it’s often the best you can do. Get it right often enough and people will believe that you too have psychic debugging powers.

Comments (14)
  1. Greg D says:

    Interesting point!  As a relatively junior dev, with only about 2 years of industry experience after 2 years in grad school, I find myself falling into this overanalysis trap more often than I should.  

    Perhaps it doesn’t help that, in so doing, I’ve also discovered a few lower-level bugs (slow handle leaks, e.g.).  The minor nature of those bugs doesn’t really justify the amount of time I’d spent spelunking that code, though.

    I may partially blame the grad school experience too, as I often had to drill down deeply through definitions, etc to understand and complete my work.

  2. Raymond Chen wrote an article in his long running series ‘Psychic Debugging’ today which…

  3. Skip says:

    As a counterpoint to this, while you should assume that the code is mostly correct, you should not assume that the comments around the code bear any relationship to the code itself.   Programmers are lazy, and many times when a piece of code is updated, the comments won’t be.

    When I’m diving into someone else’s code it’s usually a good idea to use an editor that parses the code into different color objects, and for me to set the comments color to be light grey on white.  That way I can read them if I try to, but when I’m scanning the code quickly it doesn’t interfere with my comprehension.

  4. nksingh says:

    Are there some better tools for this task than grep?  

    I tend to prefer Visual Studio or something like Source Insight which create a database of symbol definitions.  I’m just wondering if there are any good tools to examine debug info to automatically get to definitions from callsites.  

    I’m curious about how experienced folks find stuff, and hoping it doesn’t come down to a bunch of grepping.

  5. This is exactly the idea behind one of the checkers in the Stanford Metacompilation project. The paper’s title is "Bugs as Deviant Behaviour" and can be found here:

    http://metacomp.stanford.edu/derive.ps.gz

    In short, you assume that most of the code is correct and infer a belief about the library being used. When this belief is violated, its probably a bug.

  6. BryanK says:

    nksingh — ctags / etags can do that on *nix, I think.  And one or the other of those can usually be used from your editor of choice (depends on the editor though).  Not sure about "tags" files in Windows in general, though I’d suspect the functionality exists somewhere.

    I know that VS6, VS2003, and VS2005 all have "go to definition" options on the right-click menu for most identifiers.  It didn’t work all that well on C++ code (in my experience: I haven’t tried it in VS2005), but it works great in C# in 2003/2005, and VB in v6/2003/2005.

  7. This is exactly the idea behind one of the checkers in the Stanford Metacompilation project. The paper’s title is "Bugs as Deviant Behaviour" and can be found here:

    http://metacomp.stanford.edu/derive.ps.gz

    In short, you assume that most of the code is correct and infer a belief about the library being used. When this belief is violated, its probably a bug.

  8. Michael Puff says:

    But sometimes the obvious is overlooked just because it is obvious. Once a colleauge asked me for help because he got an access violation everytime he would run a particular code. At first glance the code was OK. And the code was OK. But we kept gettimg those odd access violations. In the end it turned out that he was using an object he hadn’t created yet.

    That taught me two things:

    1. Don’t start looking at the code from the middle.

    2. First check the most simply failour reasons. ;)

  9. Norman Diamond says:

    The theory behind this heuristic is that

    whoever wrote this code has a better

    understanding of how it works than you do.

    (This is a pretty safe bet since your

    knowledge of this code is approximately zero.)

    thedailywtf.com knows better (even if they don’t know why their original name was better ^_^)  I envy people who can work in an environment where your heuristic works.

    Monday, April 23, 2007 3:55 PM by nksingh

    Are there some better tools for this task than

    grep?

    findstr?  ^_^

    Monday, April 23, 2007 4:49 PM by BryanK

    I know that VS6, VS2003, and VS2005 all

    have "go to definition" options on the right-

    click menu for most identifiers.  It didn’t

    work all that well on C++ code (in my

    experience: I haven’t tried it in VS2005),

    You’re not missing much.  It works "sometimes" in VS2005 too, including SP1.  Browsing all references works sometimes too, and it really is useful when it works.

  10. cmov says:

    @Skip: That’s why I tend to use *no* comments when I write something. Good code documents itself. And if that makes it hard to read, well, it was hard to write too…

    I *do* comment the places where I’m not certain about the functionality, and put in the comments what it should do and why the attempt to achieve something was written the way it was written.

    If I’m in a "comment mood" and add comments to places where I’m certain it works OK (for others to read when they feel like it), I *never* put in comments what can be found in man pages or on MSDN, etc. Standards can change over time anyway.

  11. Tom M says:

    nksingh, have you tried Visual Assist X from http://www.wholetomato.com/, because it has some really good goto definition functionality, amongst other things.

  12. andy says:

    nksingh: you’ve also got head, tail, less/more at your disposal :)

    Once you’ve found a couple of interesting files use e.g., less which allows you to search inside the file (‘/’ is the command..)

    Visual Studio has a nice "Find in files" function btw. Believe Eclipse has a similar feature.

  13. MGR says:

    Usually I ask the developer to show me the portion of code were he/she believes the bug can be. Then go thru that code roughly asking my doubts to them. Then I go for those area’s where they have NEVER thought even to look for the bug. In most of the cases the bug occurs, were they say, there is no chance of finding one!!!

Comments are closed.