[Development] when to Extract Method (was Re: commas in ctor-init-lists)

[Marc Mutz]

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.mutz at kdab.com> | Senior Software Engineer
KDAB (Deutschland) GmbH & Co.KG, a KDAB Group Company
Tel: +49-30-521325470
KDAB - Qt, C++ and OpenGL Experts