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

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


More information about the NetworKit mailing list