[Networkit] Code Quality

Marvin Ritter marvin.ritter at gmail.com
Wed Aug 20 02:49:12 CEST 2014

@Michael: It was really just the first tiny example that my mind came up
with, but nice that you fixed it anyway.

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

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>

> 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/a512d688/attachment.html>

More information about the NetworKit mailing list