Review Request 123178: [KCompletion] Delete condition inside KLineEdit::keyPressEvent that is already checked in a former condition

[David Faure]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123178/#review78469
-----------------------------------------------------------


I agree with removing the redundant if(), obviously.

I don't agree that removing the "else"s makes the code more readable. One has to spot the "return" in every branch to realize that only one of these branches will ever be executed -- which is no longer apparent from the overall structure. I vote for keeping the "else" and reducing this patch to the minimal version of it, just removing the redundant nested if().

- David Faure


On March 29, 2015, 10:23 p.m., David Gil Oliva wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123178/
> -----------------------------------------------------------
> 
> (Updated March 29, 2015, 10:23 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kcompletion
> 
> 
> Description
> -------
> 
> The deleted condition checks whether completionMode is different
> to KCompletion::CompletionNone. This condition is inside another condition
> that checks the same. Therefore it's useless.
> 
> Besides, at the beginning of the method there are multiple else if conditions
> that aren't actually related (they are independent and each one ends
> with a "return"). Therefore, I have cleaned up the elses and left the ifs. I
> think that way the method is cleaner and more readable (as readable as it
> can be, given the length of it and the number of conditions).
> 
> 
> Diffs
> -----
> 
>   src/klineedit.cpp 7c2d0f9 
> 
> Diff: https://git.reviewboard.kde.org/r/123178/diff/
> 
> 
> Testing
> -------
> 
> Tests pass
> 
> 
> Thanks,
> 
> David Gil Oliva
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150404/557e758f/attachment.html>