[Networkit] Code Quality

Marvin Ritter marvin.ritter at gmail.com
Wed Aug 20 10:33:55 CEST 2014


For clarification:
The code review system in RhodeCode is very simple. Commenting
lines/changesets and set a status for a single changeset is all you can do.
So if you mark a changeset as yellow/red and the authors creates a
changeset with fixes those changesets (and their reviews) aren't linked. So
if the new changeset fixes all your concerns from the first changeset you
need to go back and change it status of the first one.
We could also be lazy and not update the status of the initial review, but
then it will be harder to keep track which issues where corrected and which
not.


On Wed, Aug 20, 2014 at 1:49 AM, Marvin Ritter <marvin.ritter at gmail.com>
wrote:

> @Michael: It was really just the first tiny example that my mind came up
> with, but nice that you fixed it anyway.
>
> @All:
> If we all agree, that we should be more aware of the code quality what do
> we want to do about it?
> Of course I trust you all to write more test cases, think twice about API
> changes and keep the Graph class clear, but I also know myself ... and
> sometimes *control > trust* ;)
> So a popular method are *code reviews* and fortunately RhodeCode (the
> system that runs algohub.iti.kit.edu) has support for it. Great, right? :D
> You can comment code lines in changesets and assign a status to the
> changeset.
>
> So my suggestion would be that *every changeset to the Dev
> branch changesets should be reviewed*. Not by the whole team, but maybe
> by 1 or 2 developers that are not the authors. The reviewers then set set
> the status of the changeset to one of these:
> green: approved, everything alright
> yellow: working code, but reviewer had ideas for improvements
> red: broken code, missing test cases or big changes that should be
> discussed on the mailinglist before. There should be a discussion and
> probably also a follow up commit to address the issues
>
> *What do you think?*
>
>
> On Mon, Aug 18, 2014 at 3:45 PM, Michael Hamann <michael.hamann at kit.edu>
> wrote:
>
>> Hi,
>>
>> Am Sonntag, 17. August 2014, 10:55:43 schrieb Marvin Ritter:
>> > *1) Be more careful with changes to core classes (e.g. Graph, readers,
>> ...)*
>> > Example: Someone changed the parameter of Graph.BFSfrom (the handle gets
>> > now 2 parameters).
>>
>> Concerning the example: I've added a backwards compatibility/convenience
>> wrapper similar to what we have for the new edge iterators in the edgeids
>> branch which allows the handles to accept either one or two parameters. I
>> hadn't added that back then as I didn't know about the possibility.
>>
>> > *2) Write test good test cases*
>> > I just fixed a method in Graph and removed another one because it was
>> wrong
>> > and never used.
>> > Everybody makes mistakes, but if you write good test cases or ask
>> someone
>> > to review changes, it is more likely that we notice them before a
>> release.
>>
>> Concerning the test cases, imho good test cases should also include test
>> cases
>> with graphs with some deleted nodes and edges. As I'm filtering some of
>> the
>> graphs that I analyze, I've experienced crashes even in algorithms like
>> PLM.
>> Using "upperNodeIdBound" and paying special attention to "none" values in
>> partitions is really important (partitions don't know about the fact that
>> nodes have been deleted). I've just noticed that
>> ParallelConnectedComponents
>> simply ignored the fact that nodes might be deleted and treated them as
>> isolated nodes (I've fixed that now).
>>
>> > *3) Fix compiler warnings*
>> > I know most of the compiler warnings from gcc can be ignored, but I
>> would
>> > prefer fixing them anyway. It is not a big deal and it makes it easier
>> to
>> > find important ones.
>>
>> +1
>>
>> Michael
>>
>> _______________________________________________
>> NetworKit mailing list
>> NetworKit at ira.uni-karlsruhe.de
>> https://lists.ira.uni-karlsruhe.de/mailman/listinfo/networkit
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ira.uni-karlsruhe.de/mailman/private/networkit/attachments/20140820/3c2b54f8/attachment.html>


More information about the NetworKit mailing list