Developer Area/How to Review Code
From Mahara Wiki
< Developer AreaRevision as of 19:15, 25 May 2011 by Francois (dump my thoughts)
Revision as of 19:15, 25 May 2011 by Francois (dump my thoughts)
- If you submit a change, you can mark as verified unless you specifically want others to test it.
- If you're the first reviewer, you can +1 or +2 a commit.
- If you're reviewing something that's already +1, you cannot just give it another +1. You must take the other review into consideration and give it a +2 or you directly invite another person to review and give a +1.
- Coding guidelines should be followed unless there's a good reason not to.
- Curl: don't use directly, use htdocs/lib/web.php:mahara_http_request() instead
- No magic numbers: use constants instead of hard-coded numbers as much as possible
- No DIRECTORY_SEPARATOR: always use the UNIX path separator when opening files (it works on Windows NT too)
Langstrings / help files
- Apart from messages to the logs, everything should be translatable
- Check for grammar / typos
- Make sure there are no unnecessary DB queries being added
- Very slow operations should ideally be done in cron
- Check for cross-site scripting problems:
* watch out for |safe in templates * templates should be escaped by default
- Check for cross-site request forgeries
* forms should ideally use pieforms which has a built-in session key check * other operations that change the state of the application need to check the session key before doing anything
- SQL injections
* use placeholders in queries * ideally, do the validation of the parameters close to where they're used so that it's obvious that it's validated
New upstream components
- our patches are maintained but not mixed in with upstream code
- upstream doesn't add new dependencies
- check if there are new copyright statements (which could lead to GPL-incompatible code being added to our codebase)