Peer reviewing

Peer review check-list

These are points to consider while peer-reviewing issues. They can also be applied when solving issues, to reduce the likelihood of problems during peer-review and integration.

Syntax

Ensure that:

• there are no unnecessary blank lines in the new code;
• blank lines do not contain spaces;
• variables are named correctly (all lower case, no underscores);
• functions are named correctly; and
• there are no unnecessary changes to whitespace in other areas on the file.
• new strings Don't Use Camel Case

See the Coding style guide for details.

Output

Ensure that:

• the code doesn't use buffered output unless absolutely necessary; and
• lang-strings are used instead of hard-coded strings for text output.

Databases

If there is SQL code you can test quickly, do so.

Ensure that:

• there are minimal DB calls (no excessive use of the DB); and
• the code uses SQL compatible with all the supported DB engines.

Misc

Ensure that:

• the code seems to solve the described problem;
• there are specific testing instructions that state how, as well as what, to test;
• the code doesn't use deprecated functions (see Deprecated functions in 2.0);
• the code makes sense in the broader scheme of things (look at the whole function, not just the altered code); and
• the code is easy to understand, and where it isn't, comments have been provided.
• It doesn't break XHTML
• It doesn't remove strings, or rename strings from stable branches (permitted only in master)
• php docs have been updated and adhere to coding style guide
• The idea/solution makes sense and is complete.
• The developer has searched for and fixed other areas that may also have been affected (provided they are in the scope of the original issue)
• String changes include AMOS syntax where required (moving, copying, deleting strings)
• No inline styles have been used in HTML output (everything has to be in CSS)
• CSS has been added to the appropriate CSS files (base, specific area, sometimes canvas)
• $_GET, $_POST, $_REQUEST, $_COOKIE, and $_SESSION are never used
• Unit tests all pass for related areas where changes have been made
• Uses sesskey for all write actions where appropriate (some read actions as well although not as common)
• Appropriate labels are added if needed (Reference :- Tracker Issue Labels)
• If the fix introduce a need to alter/add something to dev docs, it must be accomplished before the issue is closed.

GIT

• The commit message includes the tracker issue number and the component. Ideal format is MDL-xxxx Component Commit message
• Git history is clean
• Make sure the original author of the work has been given credit within the commit (as author of in the commit message if changes were made)

See Also

• Code checker plugin