A more hands on review process

[Tom Albers]

Hi Stephen,

Op donderdag 31 juli 2008 20:29 schreef u:
> 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/.

Or extragear.

> 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.

I just have to disagree. ;-) I think the current system works perfectly, is your improvement based on any kind of conversations with people or is it a static observation? After applications get moved to kdereview and it gets announced, I regulary see someone fixing the build system, checking of there is a messages.sh, etc, etc. 

I think the current system works ok. If you base it on criteria, one has to wait untill all checkboxes are checked, which can take more time (read: forever), so in that case you also need to define, who checks what. I rather not go on that path. So, although your proposal is a more 'official' review and might be better, I think the current system is good enough and does it's job.

> This should result in higher quality code. 

That's very optimistic ;-)

> 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,

That's already in place in the document at:
http://techbase.kde.org/Policies/SVN_Guidelines

> 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.

That's a simplification, which is not accurate. I have not seen any proof that with this system the review process would be quicker. If it would be I'ld object to it, as I think the two weeks is also a good minimum. There are always people on holidays or very busy. 

Besides, ticking boxes does not tell anything about the quality either. An i18n review by Albert is much more valuable than a review in that area by me.

> 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).

The list below is indeed a nice list for people to look at before they start moving applications to kdereview. We should point to a page listing that from the svn_guidelines page.

Best,

Toma

> 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.