Re: [Development] when to Extract Method (was Re: commas in ctor-init-lists)
Which actually raises the question, can we get Extract Lambda refactoring
?:)
On Fri, Jun 3, 2016 at 4:59 PM, NIkolai Marchenko
wrote:
> Or you could turn the second expression into a lamba for lazy evaluation
> and use it like
> bool computationNeeded = [](){ return another.such(that, makes,
> the.line.too.long)
>|| and.then.some.more(); }
> if (alreadyComputed || computationNeeded())
>
> On Fri, Jun 3, 2016 at 4:37 PM, Olivier Goffart wrote:
>
>> On Freitag, 3. Juni 2016 12:50:08 CEST 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.
>> >
>> > 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,
>>
>> What i'd possibly do:
>>
>> bool alreadyComputed = some.really(long.and.complicated, expression);
>> bool computationNeeded = another.such(that, makes, the.line.too.long)
>>|| and.then.some.more();
>> if (alreadyComputed || computationNeeded) {
>> delete thing;
>> return;
>> }
>>
>> If the cost of calling the conditions function is neglectible enough that
>> it
>> can be done even if the first condition would be false.
>>
>> --
>> Olivier
>>
>> Woboq - Qt services and support - https://woboq.com -
>> https://code.woboq.org
>> ___
>> Development mailing list
>> Development@qt-project.org
>> http://lists.qt-project.org/mailman/listinfo/development
>>
>
>
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] when to Extract Method (was Re: commas in ctor-init-lists)
Or you could turn the second expression into a lamba for lazy evaluation
and use it like
bool computationNeeded = [](){ return another.such(that, makes,
the.line.too.long)
|| and.then.some.more(); }
if (alreadyComputed || computationNeeded())
On Fri, Jun 3, 2016 at 4:37 PM, Olivier Goffart wrote:
> On Freitag, 3. Juni 2016 12:50:08 CEST 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.
> >
> > 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,
>
> What i'd possibly do:
>
> bool alreadyComputed = some.really(long.and.complicated, expression);
> bool computationNeeded = another.such(that, makes, the.line.too.long)
>|| and.then.some.more();
> if (alreadyComputed || computationNeeded) {
> delete thing;
> return;
> }
>
> If the cost of calling the conditions function is neglectible enough that
> it
> can be done even if the first condition would be false.
>
> --
> Olivier
>
> Woboq - Qt services and support - https://woboq.com -
> https://code.woboq.org
> ___
> Development mailing list
> Development@qt-project.org
> http://lists.qt-project.org/mailman/listinfo/development
>
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] when to Extract Method (was Re: commas in ctor-init-lists)
On Friday 03 June 2016 15:14:13 Edward Welbourne wrote: > Marc Mutz > > > 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), > > ... > > Just to be clear, my example code's delete thing was just using that as > "there's some random tidy-up we need to do before any early return"; so > the three conditions are all "we need to return early from this method" > and the need to delete thing (which was nowhere referenced in any of the > conditionals) is incidental to the test. In such a case I very much > doubt there's a nice name for the method. Ok, sorry, then I misunderstood. In that case: the thing should have been held in a scoped pointer and the three guard clauses should stay separate, but with just 'return' as the action. With the cleanup dealt with by RAII, I don't see a reason to combine the ifs anymore. Thanks, Marc -- Marc Mutz | 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
Re: [Development] when to Extract Method (was Re: commas in ctor-init-lists)
On Freitag, 3. Juni 2016 12:50:08 CEST 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.
>
> 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,
What i'd possibly do:
bool alreadyComputed = some.really(long.and.complicated, expression);
bool computationNeeded = another.such(that, makes, the.line.too.long)
|| and.then.some.more();
if (alreadyComputed || computationNeeded) {
delete thing;
return;
}
If the cost of calling the conditions function is neglectible enough that it
can be done even if the first condition would be false.
--
Olivier
Woboq - Qt services and support - https://woboq.com - https://code.woboq.org
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] when to Extract Method (was Re: commas in ctor-init-lists)
On sexta-feira, 3 de junho de 2016 13:10:42 BRT Edward Welbourne wrote: > Thiago Macieira > > > Another thing to be very, VERY careful is about nested function call > > > > chains, as in: > >if (foo(bar(), baz(quux()), variable, xyz()) == variable) > > > > Can you tell me if bar, baz, quux, or xyz modify variable? If so, what > > is the call order? > > If any of bar, baz, quux or xyz plausibly might modify variable, then > this code has undefined behaviour - because the call order is undefined. Exactly! > (... except for quux() being called before baz; and baz, bar and xyz > being called before foo, of course; the point in all that at which > variable's value gets read is indeterminate, any which way.) > > Not that I see how this bears on whether that complex call to foo() > should be extracted as a separate method, -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel Open Source Technology Center ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] when to Extract Method (was Re: commas in ctor-init-lists)
Marc Mutz > 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), > ... Just to be clear, my example code's delete thing was just using that as "there's some random tidy-up we need to do before any early return"; so the three conditions are all "we need to return early from this method" and the need to delete thing (which was nowhere referenced in any of the conditionals) is incidental to the test. In such a case I very much doubt there's a nice name for the method. Eddy. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] when to Extract Method (was Re: commas in ctor-init-lists)
Thiago Macieira > Another thing to be very, VERY careful is about nested function call > chains, as in: > >if (foo(bar(), baz(quux()), variable, xyz()) == variable) > > Can you tell me if bar, baz, quux, or xyz modify variable? If so, what > is the call order? If any of bar, baz, quux or xyz plausibly might modify variable, then this code has undefined behaviour - because the call order is undefined. (... except for quux() being called before baz; and baz, bar and xyz being called before foo, of course; the point in all that at which variable's value gets read is indeterminate, any which way.) Not that I see how this bears on whether that complex call to foo() should be extracted as a separate method, Eddy. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] when to Extract Method (was Re: commas in ctor-init-lists)
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 | 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
Re: [Development] when to Extract Method (was Re: commas in ctor-init-lists)
On sexta-feira, 3 de junho de 2016 12:50:08 BRT Edward Welbourne wrote: > > 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 ? Another thing to be very, VERY careful is about nested function call chains, as in: if (foo(bar(), baz(quux()), variable, xyz()) == variable) Can you tell me if bar, baz, quux, or xyz modify variable? If so, what is the call order? -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel Open Source Technology Center ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
