Content
Code Review Guidelines
Correctness
As a reviewer, your job isn’t to make sure that the code is what you would have written - because it won’t be. Your job as a reviewer of a piece of code is to make sure that the code as written by its author is correct.
Coding Style
We try to adhere to the Ruby community styleguide. At some point we’ll have to make decisions about some rules that are not clear or defined within that styleguide. In that case, we should either fork it or note all decisions here.
Most of our code does not yet adhere to this styleguide, but we want all new code to adhere to it. You don’t have to improve existing code when making changes, but we encourage it. If you do, please do all improvements in a separate commit from the actual change, so the improvements don’t hide your actual code changes in a diff.
Before committing, please run your new code through Rubocop. It detects deviations from a lot of things in the style guide and things that are bad practice in general. You obviously don’t have to fix issues with existing code. There’s a list of editor plugins in the Rubocop readme.
When reviewing code and you think the author hasn’t run the code throug Rubocop, please ask them to.
Commit Messages
- First line: less than 72 characters, this is when GitHub shows ‘…’
- Blank line
- Detailed description of the change, wrapped to 72 characters so the text is readable in
git log
See the Git Book: http://git-scm.com/book/en/Distributed-Git-Contributing-to-a-Project#Commit-Guidelines
Testing
- All tests must be green on continuous integration
- Appropriate unit and integration tests
- e.g. test application logic via rspec tests and its integration with the user interface via Cucumber tests
- Test JavaScript code, e.g. via Jasmine or Selenium/Cucumber
- but: Cucumber tests that don’t test JavaScript must not have the @javascript tag, otherwise they run slower
- Look for sufficient coverage of both Ruby and JavaScript code
- CI will provide at least Ruby line coverage information at some point in the future
- Forms: besides testing for success response, also test whether values are actually saved
- e.g. read form or look into database
- Bugfixes: must contain a test which detects the bug they fix to prevent regressions
- Translations: Never use a specific translation string in a test. We might want to change them in the future and don’t want to fix tests when we do.
Security
Every developer and reviewer should read the Rails Security Guide.
Changelog
- As of OpenProject Stable Release 3.0.8 all changes made to the OpenProject software are documented via work packages on openproject.org. There is no
- The Roadmap view gives a corresponding overview.
- To prevent inconsistencies and avoid redundant work there is there is no additional change log in the source code.
See History of Changes section of the Release Process
Other
- For external contributions: Check whether the author has signed a Contributor_License_Agreement and kindly ask for it if not
- Copyright notice: When new files are added, make sure they contain the OpenProject copyright notice (copy from any file in OpenProject)
- Adding Gems: When adding gems, make sure not only the Gemfile is updated, but also the Gemfile.lock
- No Trailing Whitespace
- Single newline at the end of a file
Accessibility
General
- Readability and Understandability: The reviewer should understand the code without explanations outside the code.
There is never anything wrong with just saying “Yup, looks good”. If you constantly go hunting to try to find something to criticize, then all that you accomplish is to wreck your own credibility.
You shouldn’t rush through a code review - but also, you need to do it promptly. Your coworkers are waiting for you.
Citations from: http://scientopia.org/blogs/goodmath/2011/07/06/things-everyone-should-do-code-review/
http://beust.com/weblog/2006/06/22/why-code-reviews-are-good-for-you/
https://developer.mozilla.org/en/Code_Review_FAQ