A more hands on review process

[Stephen Kelly]

Hi,

One of the reasons I started documenting how kde does non-coding tasks[1] is
to see where they might be improved. First up, kdereview.

Currently kde has a review process in place for new applications and
significant additions to libraries. This process is time based, and
consists of moving new code into kdereview and announcing it on
kde-core-devel to mark the start of a two week review period. The new code
is then moved into its target module inside KDE/.

This process has worked successfully and is generally gets participation
from core-devel readers, and well received by the reviewed developer(s). A
problem I see with it though is that it's sometimes unclear who has
reviewed the code, and what criteria they used to judge it.


EmoticonsLib seems to have been reviewed quite thoroughly for retaining
copyright info, design and performance.
http://lists.kde.org/?l=kde-core-devel&m=121071679915201&w=2

KAppTemplate didn't have any replies. It's unclear if it was reviewed.
http://lists.kde.org/?l=kde-core-devel&m=120939264522403&w=2

The Okteta kdereview thread was used for discussion of kdeutils and
build/linker issues. It's not clear if the application code was reviewed.
http://lists.kde.org/?l=kde-core-devel&m=120794691503053&w=2 


I propose a review process based on review criteria instead of time.
Reviewers would 'tick the boxes' to confirm that they have reviewed it, and
what they looked for. Multiple reviewers could tick only some of the boxes
each if necessary (I'd have no idea if something works on solaris or has
security flaws for example), and together create a complete review.

This should result in higher quality code. It would also hopefully encourage
developers to put things such as design considerations and dependencies in
the mainpage.dox. Developers would also keep such criteria in mind before
moving to kdereview, and point reviewers to the information. Reviewers
could then simply confirm what the developer claims and tick the boxes to
complete the review. This could make the review process shorter, at the
expense of the developer putting in more effort to document the app/lib.

If this is seen as a good idea, I'd like to get feedback on the criteria
I've suggested below. Anything covered by krazy is probably not necessary
to be on this list (eg, licence, copyright information).

[1] http://techbase.kde.org/Development/Software_Engineering_Framework


================

Code review should be constructive and specific. These criteria are a
guideline for what reviewers should consider when reviewing new code. They
do not all have to be completed, and additional criteria can optionally be
defined by the reviewer. The most important result of a review is that the
reviewer states whether the code is of enough quality as it is *now*, or
whether changes are necessary, and what criteria were considered in making
that assessment.

General
* The application / library works and performs the expected function.
* The library / application uses existing kdelibs classes where appropriate
instead of reinventing the wheel.
* The application includes all necessary cmake checks for locating
dependencies.

Maintainability
* The application / library is designed for maintainability and is
consistent with KDE norms.
* (Libraries only) All public classes use a private d-pointer class.
* Complex algorithms and optimizations are sufficiently commented to be
understandable.
* Methods are public, protected, virtual and const as appropriate.
* Dependencies are still in active development

Security
* The application / library has no obvious security flaws.
* Network accessing protocols
* html entities ('<', '>', "'", '"', '?') are encoded

Quality
* The application / library passes all krazy tests. All exceptions in
a .krazy file are justified.
* Exception handling done right.
* Library classes are threadsafe where appropriate.
* Appropriate data structures are used ( eg, QMap or QHash etc)

Portability
* The new code has no unnecessary platform specific code, such as
reading /proc etc. This does not apply to new code which is relevant only
on specific platforms.
* The application compiles and runs on Windows, Mac, BSD, Solaris.

Documentation and comments
* All public methods are documented.
* Private methods and classes are documented where necessary and marked as
@internal.
* Comments in the code are appropriate and not excessive.
* Application user documentation is written 

Style
* Coding style in consistent with the rest of the library / application
* Variable names are consistent with the target module or library module. (m
or m_ prefix, camel casing, descriptive variable names instead of a, b and
c2 etc)
* There are no excessively large methods. Methods longer than 40 lines can
be broken into multiple methods if possible.
* There are no excessively long lines. for lines greater than 80 characters
it might be possible to use intermediate variables more.