On Friday 03 June 2016 14:50:08 Edward Welbourne wrote: > On Friday 03 June 2016 10:05:52 Edward Welbourne wrote: > >> if (somee.really(long.and.complicated, expression) || > >> another.such(that, makes, the.line.too.long) || and.then.some.more()) > > Marc Mutz responded: > > To be perfectly blunt: such expressions shouldn't be > > allowed. Period. Neither with nor without line breaks. Such monsters > > should be subjected to Extract Method with extreme prejudice. > > I'm fascinated - and we're now on a whole new topic - you mean this, > even when the relevant combination of expressions is meaningless outside > the one context in which it is used: an expression that combines three > computed values with a binary operator is so complex it should be turned > into a method ? Even if the three conditions are unrelated ? > > How about if all three are tested separately ? > > if (some.really(long.and.complicated, expression)) { > // this one reason for an early return > delete thing; > return; > } > > if (another.such(that, makes, the.line.too.long)) { > // this quite different reason for it > delete thing; > return; > } > > if (and.then.some.more()) { > // a third quite different reasons > delete thing; > return; > } > > I see lots of code like that. If combining them into > > if (some.really(long.and.complicated, expression) // this one reason for an > early return > > // this quite different reason for it: > || another.such(that, makes, the.line.too.long) > > // a third quite different reasons: > || and.then.some.more()) { > > delete thing; > return; > } > > requires Extract Method, I suspect the method name is going to end up > being too long to fit on a line, on account of being > > thisOneReasonOrThatQuiteDifferentReasonOrThatThirdReason(long, expression, > that, makes, the, and, another, some); > > with lots of unrelated parameters in order to do its job. Well, I > suppose its name could be shouldReturnEarlyFromOriginalMethod(...) > instead; but I conclude that you actually prefer the three separate > clauses, albeit in the extracted method, where each returns true, so > that the calling code only has to do its tidy-up (and possibly other) > code once.
The three clauses should stay three clauses if the action (their then-block) is independent. If the then-block, as you seem to suggest, is idenitical in tokens and semantics, then you *will* find a name to describe it that doesn't just transliterate the original C++ code into English: shouldDeleteThing, thingIsExpired, isNoLongerNeeded(thing), ... > So, I'm fascinated: when is Extract Method the right pattern to apply ? > I would normally reserve it for cases where it adds semantic clarity (by > naming the condition concisely), avoids duplication or modularises an > over-long method, To say it with Fowler: It should be used whenever the original code Smells. Overly-long if statements Smell, imo. Thanks, Marc -- Marc Mutz <marc.m...@kdab.com> | Senior Software Engineer KDAB (Deutschland) GmbH & Co.KG, a KDAB Group Company Tel: +49-30-521325470 KDAB - Qt, C++ and OpenGL Experts _______________________________________________ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development