Re: Review Request 123178: [KCompletion] Delete condition inside KLineEdit::keyPressEvent that is already checked in a former condition
--- 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 > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 123178: [KCompletion] Delete condition inside KLineEdit::keyPressEvent that is already checked in a former condition
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123178/ --- (Updated April 7, 2015, 6:41 a.m.) Review request for KDE Frameworks. Changes --- Take back "else if"s. Fix indentation. 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 (updated) - src/klineedit.cpp 7c2d0f9 Diff: https://git.reviewboard.kde.org/r/123178/diff/ Testing --- Tests pass Thanks, David Gil Oliva ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 123178: [KCompletion] Delete condition inside KLineEdit::keyPressEvent that is already checked in a former condition
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123178/#review78590 --- Ship it! Ship It! - David Faure On April 7, 2015, 6:41 a.m., David Gil Oliva wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/123178/ > --- > > (Updated April 7, 2015, 6:41 a.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 > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 123178: [KCompletion] Delete condition inside KLineEdit::keyPressEvent that is already checked in a former condition
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123178/ --- (Updated April 9, 2015, 7:57 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Changes --- Submitted with commit 7a3735e2bfcb506e820961199c37f5876a1de7fd by David Gil to branch master. 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
