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

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-pim
Subject:    Re: [Kde-pim] Review Request: Add a safe ensureParsed() method to KMime::Contents
From:       Ingo_Klöcker <kloecker () kde ! org>
Date:       2012-02-26 20:46:58
Message-ID: 20120226204658.15155.74386 () vidsolbach ! de
[Download RAW message or body]

> 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@kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[prev in list] [next in list] [prev in thread] [next in thread] 