On Removing an Element from a List
by Dominik • March 17, 2012 • Developers • 6 Comments
Recently, there was a very good blog about The Importance of Mentoring. It was mentioned that the 3200 slocs of the gsoc projects could be cut down to 500 slocs, doing exactly the same thing.
While hunting some crashes in the new code folding code from the last gsoc project, I obviously had a closer look on how things are implemented. The code folding uses a simple tree structure. Nodes can be added, removed or moved around. So I stumbled over the following code:
01 // removes it from children list (QVector)
02 bool SomeClass::removeItem(Item* it)
03 {
04 bool found = false;
05 int i;
06
07 for (i = 0 ; i < children.size(); ++i) {
08 if (children[i] == it) {
09 found = true;
10 break;
11 }
12 }
13
14 if (found) {
15 children.remove(i);
16 return true;
17 }
18
19 return false;
20 }
This code appears several times in different locations. It can easily be rewritten as
01 // removes it from children list (QVector)
02 bool SomeClass::removeItem(Item* it)
03 {
04 int i = children.indexOf(it);
05 if (i != -1) {
06 children.remove(i);
07 }
08
09 return i != -1;
10 }
So without much work, the code is reduced to the half. Diving further into the code, I stumbled over a class KateDocumentPosition. This class is a line/column tuple representing a position in the document. It features operators like <, >, for convenience. Now you may guess it: Kate Part is a text editor using “document positions”all over the place, e.g. for the cursor, the text selection, bracket matching, search & replace and what not. In fact, there is no way around using line/column tuples as position markers. Thus, it should not be surprising that we have a public class called KTextEditor::Cursor, featuring everything what KateDocumentPosition implements (and more). The Cursor class is basically used everywhere, and it works as expected (our unit test rely on it, too). There is no need to duplicate the code. This probably happened because the student was not aware of it.
Martin writes “Be prepared for the worst“. Well, true. In this case, the project was successful and the new code folding works better than the old one (after fixing the crashes). Now if you are a gsoc student reading this blog, don’t feel discouraged. Rather feel encouraged to communicate with the developers a lot, e.g. by discussions on the mailing list 
What about the one-liner
children.removeOne(it);
?
Don’t forget children.removeTwo(Item one, Item two).
Erheheh, funny
http://qt-project.org/doc/qt-4.8/qlist.html#removeOne
Right, I should have mentioned: it’s a QVector.
Or, rather, get into the habit of submitting every piece of code you produce for review. Before submitting the code for review, do make sure it builds and works fine. Do not submit debug statements, auxuliary comments intended only for yourself and other dirt. Pay close attention to whitespace and const correctness.
Also, get into the habit of collecting othere good habits and practices. Gt yourself a notebook and write those down as you come across them. If you tend to forget about things (like I do), make a huge poster, like “CONST CORRECTNESS!! GIT DIFF –CHECK!!” and hang it over your desk.
And, most importantly, don’t fall into the trap of thinking that Open Source != professional, high quality code and quick hacks are acceptable. They’re not, and, after all, KDE is not a completely open (as in messy bazaar) project – there’s a whole commercial ecosystem around it.
I totally agree with this. No code of GSOC should ever be allowed to enter git without being reviewed first. Students should understand that their code is going to be used by millions of people and will probably remain in KDE software for a very long time, so messing around is not acceptable. Of course, students still need to learn, so they should not hesitate to submit their code to review, but the review should be done thoroughly (and in the first place by the mentor).