[Kde-pim] Review Request: Add a safe ensureParsed() method to KMime::Contents

[Ingo Klöcker]

> On Feb. 25, 2012, 8:51 p.m., Ingo Klöcker wrote:
> >
> 
> Ingo Klöcker wrote:
>     ReviewBoard or my old Konqueror has eaten the following comments:
>     
>     Did you write unit tests for this? Is everything else already checked with unit tests? Do they pass? If any of those questions is answered with no, then I'm not confortable with your change. In particular, because you seem to have missed a few spots.
> 
> Szymon Stefanek wrote:
>     Hum.. I didn't write unit tests nor I have run the existing ones.
>     I'm just trying to fix a bug that makes me nervous :D
>     
>     This change doesn't modify the behavior of any existing function though. In fact
>     it was explicitly written this way to avoid touching any existing code path.
>     
>     I might write an unit test for the new function later, when I find some
>     time to figure out how it has to be done...
>     
>     Having said this, I'm certainly willing to fix the issues you've found
>     and thank you for looking at this patch :)
>

Granted, your changes do not modify the behavior of existing functions. This does not save you from writing a unit tests for the new function. Do it now or wait for somebody else to approve your patch. If you don't write the test now you'll never going to write it. The point of the unit test is not to prove that your function works now. (For this you should have written the unit test before you implemented the function.) It's to make sure that it does still work as intended 10 years from now.


> On Feb. 25, 2012, 8:51 p.m., Ingo Klöcker wrote:
> > kmime/kmime_content.cpp, lines 255-258
> > <http://git.reviewboard.kde.org/r/104078/diff/1/?file=51131#file51131line255>
> >
> >     Why not simply
> >       if ( !d->parsed ) {
> >         parse();
> >       }
> >     ?
> >     It's shorter and easier to read.
> 
> Szymon Stefanek wrote:
>     Haha.. well.. it can be written this way too.
>     I don't mind using your version :)
>     
>     <nitpicking>
>     Your version of code could be shorter if the brackets were removed but a (questionable)
>     coding convention forces you to use them with a single line instruction.
>     If you remove space in mine the number of lines of code in the two snippets is the same.
>     
>     For the "easier to read" part I'd say that it's a matter of opinion and habits... but:
>     
>     - Your version has one additional level of indentation.
>       In these years I've worked out that readability decreases with indentation level. This is
>       because at every new level you're pushing one additional piece of information on your
>       brain's stack. You will pop off this piece of information when you exit the scope.
>       The unindented version, instead, tends to "narrow down" the situation by removing 
>       superfluous information. With less indentation you generally keep less information in
>       your head. Obviously this isn't really visible with such a short piece of code
>       and not every piece of code can be written both ways.
>     
>       Another funny thing with indentation is that kdepim uses two spaces to indent...
>       I'm a tab guy so I try to avoid indenting :P
>       ...and the open bracket on the if line... aaaargh! :D
>     
>     - Your execution path has a visual jump inside.
>       If you try to follow the "already parsed" case then you enter the condition, evaluate
>       it to false and then jump to the end of the block (which often, in poorly indented code
>       is hard to find). My code has no such jumps.
>     
>     - Your condition is more complex. I know that the compiler has a single instruction
>       to handle it but the brain doesn't: it still has to read d->parsed and apply the "not".
>       Also in most interpreted languages (and in several compiled ones with optimization turned
>       off) your condition is slower to evaluate.
>     </nitpicking>

The coding convention requires brackets for a very good reason. If you have not yet experienced this reason yourself then you either had luck or are too young. I have already found more than one bug that was caused by missing brackets. FWIW, I also dislike the open bracket on the if line. I'd have put it on the next line if I had had the choice.

I'm not going to comment on your other comments because they are mostly related to personal taste.


- Ingo


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104078/#review10901
-----------------------------------------------------------


On Feb. 25, 2012, 10:57 p.m., Szymon Stefanek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104078/
> -----------------------------------------------------------
> 
> (Updated Feb. 25, 2012, 10:57 p.m.)
> 
> 
> Review request for KDEPIM and KDEPIM-Libraries.
> 
> 
> Description
> -------
> 
> This patch adds an ensureParsed() method to KMime::Contents.
> Unlike parse(), ensureParsed() can be called multiple times without
> breaking the message body.
> 
> So in turn users of KMime::Contents that do not know if parse() has been
> called on the message they are handling can always call ensureParsed().
> 
> This is a part of a larger fix for bug 291171
> 
> 
> This addresses bug 291171.
>     http://bugs.kde.org/show_bug.cgi?id=291171
> 
> 
> Diffs
> -----
> 
>   kmime/kmime_content.h 05f67c2 
>   kmime/kmime_content.cpp 37ca474 
>   kmime/kmime_content_p.h f09d293 
> 
> Diff: http://git.reviewboard.kde.org/r/104078/diff/
> 
> 
> Testing
> -------
> 
> Compiled and tested. Along with another patch to kdepim it fixes bug 291171.
> 
> 
> Thanks,
> 
> Szymon Stefanek
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/