GSoC application review - reimplementing personal metadata importers

[Konrad Zemek]

On 25.04.2013 12:34, Matěj Laitl wrote:
> Oh, nice! Do mention it. I may be stating the obvious, but I felt it 
> needs to be said: Please note that "ideal" code for a typical FLOSS 
> project may be very different from "ideal" code for a programming 
> contest. In Amarok, we try to keep code as simple (to read & 
> understand) as possible, even if it would mean it is let's say 15% 
> slower. Keep in mind that many readers of our code may not be contest 
> finalists - but we still want them to be able to understand and 
> perhaps modify it. This of course doesn't mean you should be using 
> O(n) algorithm when O(log(n)) algorithm exists or refrain from using 
> private inheritance where it is appropriate. It means "oh, this is a 
> super-smart code, but perhaps it can be expressed more clearly using a 
> line or two more?" mindset.

Now I feel that I need to respond to that. :-)

I'm a mature programmer. I have well over five years of real programming 
experience (not the home-grown kind), and in that there's over half a 
year of commercial experience. I'm not claiming I'm very experienced - 
obviously five years is a lot less than ten or fifteen, but still that's 
enough to have both "fastest code" and "shortest code" phases well 
behind me. I favor clean, easily understandable and maintanable code, 
making (sensible) sacrifices of speed and linecount. I've also 
programmed in assembly a bit and analyzed how compilers work, so I'm 
very much aware that modern compilers more often than not optimize the 
code that makes its intentions clear to be actually faster than 
hand-optimized code doing the same thing.

That's basically what I've done in my first (of many, hopefully) review; 
the cleanup wasn't to make it faster, it was for it to be understandable 
(e.g. I myself had problems with figuring out what value is actually 
returned), and therefore of better quality.

And to touch on the private inheritance. ;-) Although it's seldom used, 
it actually makes code clearer and gives out more information at a 
glance than composition. (Of course composition should be favored over 
inheritance, but inheritance should be used where appropriate.) The 
thing is to read it the right way: while you'd read "A: public 
BaseClass" as "A is a BaseClass", private inheritance would be read as 
"A is implemented in terms of BaseClass". So now that very single-line 
declaration gives you information that underneath A is built on top of 
BaseClass - and all of that without looking through A's actual 
implementation to see that A is a BaseClass wrapper. It's all about 
semantics and choosing a language construct that best represents what 
the code is supposed to do.

> Matěj 

         Konrad