Remember that the purpose of code review is to learn from the feedback of your peers and give constructive feedback to help one another improve. Even when it's not your code being reviewed, you can often pick up a lot of ideas from the discussion. When it is your code being reviewed, keep in mind that software is neither done nor perfect -- there's always something we can do better.
- Clone the project -- project owner should make sure beforehand the project is pushed to github
- Setup dependencies (
bundle, create DBs, etc.) -- A good project README will have clear instructions for what steps are necessary. - Use the project (5 minutes) -- If it's a CLI try working through the commands a few times. If it's a web app, click around and try to follow a few different "user paths" through the app. Run the project. The project owner will probably need to serve as a guide here, but the goal is to quickly get a broad understanding of what the app actually does.
- Run the test suite (5 minutes) -- Run the project test suite. It should pass and there should not be an excessive number of skipped tests. If there are failures, take a look at the failing tests and discuss what would have been needed to make them pass. If there are issues running the test suite, figure out what is needed to make it run, and discuss what steps need to be added to the README to cover this setup.
- Initial code reading (5 - 10 minutes) -- Quickly read through the all the files in the project. Don't get bogged down on any particular file, but try to get a feel for the general structure of the project. Are there many files? Few? Is the directory hierarchy flat? Deeply nested? Are the files of roughly consistent length? Does there seem to be a rough balance of source files to test files? How many different "categories" of objects seem to be present?
- Test suite review (20 minutes) -- Read the tests. Flipping between a test file and a code file is fine at this point, but try to focus on the tests themselves. Look for good testing habits like:
- Are there enough tests? Do they seem to sufficiently cover the relevant source code? Generally all public methods of a class should be covered.
- Is there attention to edge cases in our test coverage? Are we doing more than just the "happy path"?
- Do our unit tests focus consistently on the subject at hand? (Avoiding excessive use of associated objects, testing other objects' functionality, etc)
- Does the testing stack show good balance between unit, functional, and integration layers? If one layer seems to be covered much more heavily, consider what conditions might have pushed the developer in that direction. Can we think of any techniques which might help compensate for this?
- Source code review (20 minutes) -- Finally time to look at the code. By now the previous steps should have given us a some idea of what to expect from the source code. Do these expectations turn out to be correct? The list of things to look for in a code review is numerous, but here are some of the biggies:
- DRY - Is there repeated code throughout the project?
- SRP - Do classes in the project have clear roles to play within the overall system? Look for methods or other chunks of code which could represent concepts in their own right. Can we come up with additional classes to extract?
- Naming - Are methods, classes, and variables named consistently and descriptively?
- Method length - Are there many long methods? Rails controller actions and CLI flow-control methods can be especially prone to this type of bloat. Take a crack at splitting long methods into multiple, well-named methods.
- Style - Lines under 80 characters? No trailing white space? Consistent use of 2-space indentation?
- Method arity - Are there methods that take a lot of arguments? Under 4 is a good guideline. 1 or 2 is better.
- Version Control Usage -- Are there a good number of discrete, well-packaged commits? Are the commit messages informative? If the project is a group project, does it show good usage of pull requests?