Niklas, Aaron S., Siebrand, Santhosh, Amir, Alolita had a discussion about code review. Here are the notes. It might repeat some things Sumana sent earlier and you might not agree on everything.

Problems: * Having tags in Gerrit would be nice. Commit summary lines are sometimes abused for this. * Changes and rebases are combined. * We are concerned that dashboards will become private in future. Currently it's possible to see everyone's dashboards. * Time is wasted scanning for stuff to review in different Gerrit listings. * Unowned code rotting around. Result of bad bus factor. * There is no stylize.php for JavaScript. * Lack of tools to effectively debug PHP code without print/var_dump/error_log etc.

Notes: * Not always testing manually. Depends on how big failure can be and whether there are good unit tests for it. * Drafts-feature is (sometimes) good for work-in-progress, but tests need to be triggered manually by adding Jenkins to reviewers and triggering new patchset. * We own/watch certain extensions and review new patches against them. * JavaScript code needs more documentation than PHP. * Sadly, automatic code formatting tools are rarely used. * Post-commit review does not yet have a process, but cases seem to be relatively rare. * We love the continous integration systems running our phpunit tests, looking forward to QUnit tests too. * Use wfProfilneIn/Out for things like reading or writing to files, shell calls, http calls.

Our quick tips (what we usually complain in code review) * Commit should message describe what and why. What was the problem? How does the fix resolve it? How to test that it actually works? * Commits should come with unit tests when possible. * Make methods public/protected/private (think what makes sense). Don't just make everything public! * Follow coding style (whitespace is important). * Spelling mistakes. * Avoid long functions (100+ lines). * Document all methods (even if protected) in general. Describe what they do rather than documenting the obvious @param $title Title Title object. * New public methods and classes should have @since tags. Extension developers will love you. * Avoid saying "patchset x: ..." in the commit summary, use gerrit comments instead * Use type hinting when applicable * If you need some additional functionality of a core module (or you need a function that does something similar but a different), actually improve the core module. Don't just copy+paste and modify the code * in another place. * Avoid static "inheritance" (late static binding) unless it's perhaps for a factory function * Smaller commits are easier to review * Extensions that assume direct file system access for shared storage can't be used on WMF * Refactor code as changes are made (but use separate commits if the refactoring is large). Don't let the code keep getting worse with each change.

-Niklas