What kinds of things do you look for when reviewing someone else's code before a code review/inspection?

Question

Do you look for anything in particular, or just read the code carefully and just find what you find?

Answer 1

Nothing. I just make sure to have enough comments (and not too many)

It's no shame if someone in the review finds a "better" way, or a minor bug, as long as the code is designed well.

It's not about getting "caught" making a mistake, it's about the best code you can write.

Answer 2

It would help if the group working togheter came up with a common checklist of things to look out for.

Answer 3

• (when necessary) deallocation after allocation
• at least some attempt at comments
• tombstone (copyright, author, etc at the top)
• no // TODO: auto-generated method
• closure of resources that were opened, like sockets, file handles, serial ports, etc.
• no empty try-catch(Exception) blocks
• no try-catch(Exception) blocks, generally -- they should catch a specific exception type
• proper usage of try-finally

I don't generally put too much effort into reviewing someone's algorithm. That stuff should have been vetted before s/he got started on the code in the first place.

Answer 4

I normally check for proper resource cleanup and sensible symbol naming.

I actually prefer zero attempt at comments over poor use of comments, though beneficial uses will win praise. I also know this is highly subjective, and some of what I consider to be antipatterns have been enforced over my objections as a coding standard at some sites.

Good uses:

• Citing a source, preferably including a URL.

  // Implements Bob's Algorithm, see http://example.net/algorithms/bobs
  foreach(var item in Items) ...
  

• Explaining rationale behind a decision.

  // Implemented a sort here instead of using ArrayList.Sort because
  // blah blah blah blah ...
  internal void Sort(...)
  

• Doc comments on interface members

  /// <summary>Bips the supplied foo.</summary>
  void Bip(Foo foo);
  

Bad uses:

• Using comments to explain identifiers that aren't self-explanatory - fix the identifiers instead

  // Remove all items past the expiration date the host set from the history.
  void RemoveItems() { ... }
  

• Using comments to mark bug-number, change-date or author - available in source control

  // 2008-06-20 jrandomhacker Changed if-condition to fix bug #120
  void DoItToIt(Message input)
  {
      if (input.Generator.BeginsWith("Fubarco"))
      {
          // fix for working with Fubarco servers - see bug #121
          ...
      }
  }
  

• Belaboring the obvious

  total += 1; // increment the total
  

• Commenting every few lines, whether or not the comments are helpful. This is more of a code smell, suggesting that whatever it is the code depends on needs to be refactored.

Answer 5

I am of the opinion that if you are actually reviewing code, you cant just use a checklist to do it. You need to get down deep into every nook and cranny and ask the question, could this have been done better. I use tools and tests to check for resources leaking, so human eyes are of little use there. the only broad checks I have are, code duplication and overuse of inheritance.

Answer 6

It depends on the goal of the review.

Sometimes the goal is to find bugs. (Maybe always?) Sometimes it's a security audit, in which case you might be "blind" to certain kinds of problems in the interest of getting through more code more quickly.

Or perhaps it's a review for a certain type of problem such as a race condition or incorrect error clean-up. That kind of review can be fast and very effective.

Sometimes the goal is to teach a newbie basic programming skills, or bring a new hire into the fold, showing her where all the acorns are buried.

Answer 7

Most (almost all?) of the code reviews I do are security focused, however most of these principles - and most of the details - are still applicable.

• Decide in agreement ahead of time, what the purpose of the review is, what it's focus should be, and what types of things are NOT of interest (e.g. variable names, the One True Brace form, etc. - more as a general guideline, not specific items).
• Though most of the answers here say otherwise, I DON'T agree that the purpose of code reviews is The Best Possible Code You Can Write - but the best code that it's WORTH writing. That is, we might be able to slightly refactor the code and improve it infinitesimally, but it would 6 weeks to do so - if the benefit is minimal, its just not worth it.
• Pursuant to the first two points, set a time limit, to ensure you don't get carried away and drilling TOO deep - this forces you to focus on the important stuff.
• As I said, security related - threat model your app (whether formally or ad-hoc) before CR, to point out the areas to give extra focus to. I guess this might work in a non-security format also...

As to specific things I look for:

• Data validation (not just user input, also other sources of data)
• Proper database access
• Output encoding (especially if this is a web app)
• Exception handling
• Any use of cryptography - one of the most complex programming options, and one of the most difficult to use correctly.
• Authentication and authorization mechanisms
• Specifically identified sensitive algorithms and business processes
• Network access
• Language- and technology specific issues (e.g. buffer constraints, format strings, and "evil" functions for C++)
• Dynamic code
• If there are credit cards, or other secret/private information, track all usage thereof, to verify no leakage

I said it's usually security for me, didn't I? ;-) This is by far not a complete list, but this should give you a feel for the types of issues to look for - dependant on the purpose and focus of the review.