Simple Code Review as a part of Technical Interview?

Question

We will be conducting interviews in the very near future. As I find asking technical questions to be a poor gauge of whether a person actually knows something, or just "studied" for the interview, I was thinking of conducting a simple code review during the interview. Overall I would try to keep the exercise to 5-10 minutes max.

Specifically, the format that I am thinking of using is to have two small classes (~20 lines perhaps, one in C# and one in JavaScript) that will include 5-10 "issues". These issues may range from simple things like poor variable names, to missed dispose calls on an obviously named MyDisposableClass object or something to that effect and up to things like modified closures or threading issues. I think I would likely use two samples, one for C# and one for JavaScript (since so many people say they know it and really don't).

My goal is to see how the person approaches the task, how they explain their thoughts on what is good/bad with the code and ultimately, see what issues they identify (idea being some should be targeted towards junior, intermediate, senior etc).

Has anyone conducted this type of exercise in an interview? Did you find it worked well for you? Any pitfalls that I should be aware of? Why types of "issues" do you think would be appropriate to include in the code?

As always, your thoughts are greatly appreciated. Thanks!

Edit

Awesome feedback everyone; thanks!

I spent 30 minutes this morning putting together my JavaScript "code review" code; I am not including any details on what is wrong with the code (there are some petty items, but also some major design issues). Any feedback would be appreciated

function CustomEvent(obj) {
  _handlers = new Array()

  this.subscribe = function (fn) {
    var suscribed = null

    _handlers.push(fn)
  }

  this.unsubscribe = function (fn) {
    var temp = []

    for (index = 0; index < _handlers.length; index++)
      if (_handlers[index] == fn)
        temp.push(function () { _handlers.splice(index, 1) })

    for (index = 0; index < _handlers.length; index++)
      temp[i]()
  }

  this.run = function (e) {
    for (index in _handlers) {
      try {
        _handlers[index](obj, e)
      }
      catch (ex) {
        Log.error(ex) //Assume Log is a global static utility class
        unsubscribe(_handlers[index])
      }
    }
  }

  this.dispose = function () {
    _handlers = new Array()
  }
}

C# Class to be added later today when I have some time to put it together.

Answer 1

I was given such a task in an interview and found that, as an interviewee, it offered a great opportunity to show what I know and how well I can communicate ideas. I'd highly recommend it and have used it myself when I was the interviewer.

Also don't forget to leave part of it incomplete and have them write the code to finish it. After all, you're not hiring them to just perform code reviews. Most likely they'll be spending far more time coding than reviewing.

Answer 2

This is good.

A pitfall: Including annoyingly trivially small issues Don't include issues like that would be easy to miss depending on what medium you're using to show them the code, but that wouldn't be indicative of their skill level. For example, don't include subtle syntax errors in your code. If the see them, it may make them feel as though you think they're stupid, and if they don't it could mislead you in your judgement about them.

Suggestion: include subjective 'issues' I would include some subjective code that could be argued for either way (keep it/change it). If they mention it as an issue, play devil's advocate. Otherwise, mention it yourself (you could say to them something like "you didn't mention w, but some people would think it's an issue because of x, y and z. What do you think?") and offer counterpoints to whatever they say after. This is a good way for you to test their communication skills, and how they react when under pressure. Careful not to be too harsh when debating the issue with them however - just offer counterpoints and see what they say.

Answer 3

I had an interviewer do this to me once, and it went horribly. If you do want to do this (and I think it is a good idea if executed properly):

Give code samples in a language they know

If you're requiring C# then obviously having the code sample in C# is fine; otherwise, pick a language they say they know on their resume and write the samples in that. Do not (hypothetically) tell someone that knows C++ and Java "don't worry, C# is close enough, you'll be able to tell". Half the problems in the code sample were in no way apparent to a non-C# programmer. Unfortunately I can't remember most of them now, but here's a Java example:

Object a = foo();
Object b = bar();
if(a == b)
    // do stuff

For a Java programmer there's an obvious guess; it should read if(a.equals(b)) instead. However:

1. The code presented isn't necessarily wrong. The interviewee can't know from that snippet if you intended to compare actual pointer equality or meant to use .equals()
2. This is only obvious to a Java programmer. A C++ programmer would rightly have no problem with this, as == is overloadable in C++

Here's another one I just remembered because it came up in another SO question:

void foo() {
    std::fstream file("foo");
    file << "testing" << std::endl;
}

The problem is forgetting to close file before returning, but file's destructor does that automatically -- in C++

Make your problems serious

80% of the problems I did find started with me saying "Wait...it's not __, is it?". We then launched into a debate about whether that particular thing was a problem or not, which pissed him off enough that I knew there was no way I was getting a job offer. Your problems should be epic failures that would actually break stuff, not a minor point that you could see a developer actually doing in your code base and not correcting. For example (this one I actually do remember from the interview):

String a = "foo";
a += "bar";

is not a good example. I finally realized he wanted me to say it was inefficient and I should use StringBuilder (or whatever the C# equivalent is), but for such a comically minor case there's no reason to and nobody ever really would, plus in Java it's done automatically. If the bug is a logic error, like a mistake in an algorithm, there needs to be a comment saying what that code is supposed to do, or it should be painfully obvious from the function name. For example:

int div(int n, int d) {
    return n/d;
}

This could have a bug in that non-even division will be truncated, as opposed to:

double div(int n, int d) {
    return ((double)n)/d;
}

On the other hand, people do integer division every day in code, and it's not clear from that code snippet that the intent was to return an exact quotient. Interviewers write snippets thinking "haha, this would be a silly mistake to make", and don't stop to read the code pretending not to know what the problem is to see if it might have legitimate uses

Answer 4

I routinely give a question like this for our phone screen. Our in-house recruiter emails the candidate an hour or so before the call, and we discuss results on the phone.

There are no syntax or trivia issues in the example I give, but some serious problems - a bad return value, failing to null-terminate a string, a flawed algorithm, etc.

I look for a couple things

1. What did the person find? There are probably 7 or 8 big glaring things wrong with these 10 lines of code. If someone finds 5 or 6, fine. The superstars find all of them, and some I hadn't thought of (hey, you could have integer overflow here). The chumps find 2 or 3.
2. But here's the thing - you could ask your smart friend to help you, and I'd have no way of knowing. So I ask follow up questions. "Oh, there's a buffer overflow here - how would you fix that?" Or "you say this code isn't thread safe - is it okay if you're just running in one thread?"
3. Personality match. At my company, we expect people to be able to figure things out. If the example I give is a function called "ReverseString" and the person says "well, it's really not clear what the function is supposed to do, and I'd need someone to give me complete requirements to be able to go further" that person wouldn't be a good match. If they make reasonable guesses "it looks like it's supposed to reverse a string, but it won't actually work, because there's a bug," they fit in.
4. Trade offs. There are some deliberate design flaws. "How would you fix this problem?" "What other ways are there to fix that same problem?" Ideally, someone can think of options, and make a good decision "well, I'd consider doing X, but the problem is foo, so I really want to stick with Y." A fair number (20%) of the people who fail out here (of the ones who make it this far). "I'd do it this way." "Are there any other ways?" "No, I'd do it this way." "Can you even think of any other ways?" "No, this is the right way to do it."

Answer 5

I had an interview like this once for a coop job. I did not think that it gave me a good opportunity to showcase my skills.

The code samples were in C and C++ neither of which I had thought about since our one class in C.

If I had had a warning before the interview that this the interview would consist of this I am sure that I would have done fine. Having worked in Java for the last two years it is hard to switch back to C/C++ mode in 30 seconds.

Either:

1. Have 5-6 languages available and let the interviewee choose the language (you will still get to discuss, see their train of thought etc.)
2. Or give them warning before the interview that they are going to be asked specific questions and they should brush up