[numbers] Code blocks in test methods

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[numbers] Code blocks in test methods

Heinrich Bohne
I have been asked to request some feedback on this pull request:
https://github.com/apache/commons-numbers/pull/36– specifically, about
the introduction of code blocks in the commit "NUMBERS-100: Reduce scope
of local variables".

I had the idea with the code blocks when I wanted to add a test to the
method testAdd() but was intimidated by the huge wall of code contained
in the method. When taking a closer look, this code wall is actually
composed of several test cases that are completely independent of each
other, but because the local variables live throughout the whole method
and are re-used in almost every test case, this is not obvious. The more
variables are involved, the closer you have to look to figure out which
sections are independent of the rest.

I think that, with the code blocks, it is instantly obvious that a
specific section does not depend on anything that happened before it, or
that it does not affect anything that comes after it. So I think that
they are preferable to the previous version of the file.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [numbers] Code blocks in test methods

Heinrich Bohne
Sorry, I messed up the link to the pull request:
https://github.com/apache/commons-numbers/pull/36

On 6/12/19 3:00 PM, Heinrich Bohne wrote:

> I have been asked to request some feedback on this pull request:
> https://github.com/apache/commons-numbers/pull/36– specifically, about
> the introduction of code blocks in the commit "NUMBERS-100: Reduce
> scope of local variables".
>
> I had the idea with the code blocks when I wanted to add a test to the
> method testAdd() but was intimidated by the huge wall of code
> contained in the method. When taking a closer look, this code wall is
> actually composed of several test cases that are completely
> independent of each other, but because the local variables live
> throughout the whole method and are re-used in almost every test case,
> this is not obvious. The more variables are involved, the closer you
> have to look to figure out which sections are independent of the rest.
>
> I think that, with the code blocks, it is instantly obvious that a
> specific section does not depend on anything that happened before it,
> or that it does not affect anything that comes after it. So I think
> that they are preferable to the previous version of the file.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [numbers] Code blocks in test methods

garydgregory
In reply to this post by Heinrich Bohne
I've used code blocks in this style in the past but...

(1) It is helpful to add a // comment for each block, otherwise, it feels
anonymous and weird to me.
(2) Why not refactor and pull-out methods? This then forces you to _name_
the methods, instead of the above (anonymous blocks vs. commented blocks.)

Gary

On Wed, Jun 12, 2019 at 9:00 AM Heinrich Bohne <[hidden email]>
wrote:

> I have been asked to request some feedback on this pull request:
> https://github.com/apache/commons-numbers/pull/36– specifically, about
> the introduction of code blocks in the commit "NUMBERS-100: Reduce scope
> of local variables".
>
> I had the idea with the code blocks when I wanted to add a test to the
> method testAdd() but was intimidated by the huge wall of code contained
> in the method. When taking a closer look, this code wall is actually
> composed of several test cases that are completely independent of each
> other, but because the local variables live throughout the whole method
> and are re-used in almost every test case, this is not obvious. The more
> variables are involved, the closer you have to look to figure out which
> sections are independent of the rest.
>
> I think that, with the code blocks, it is instantly obvious that a
> specific section does not depend on anything that happened before it, or
> that it does not affect anything that comes after it. So I think that
> they are preferable to the previous version of the file.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [numbers] Code blocks in test methods

Heinrich Bohne
 > (2) Why not refactor and pull-out methods? This then forces you to _name_
 > the methods, instead of the above (anonymous blocks vs. commented
blocks.)

I did not pull out the code sections into separate methods because I had no
intention of re-structuring the whole class. I only wanted to fix a bug in
the class Fraction and add a test case in FractionTest that would have
failed
due to this bug, and in theprocess organize that which was already
present in
FractionTest a bit better, because it has been pointed out to me that new
contributions should not only strive to improve functionality but also
readability.
Introducing those code blocks seemed like a straightforward way of
making the
mess in the bodies of some of the test methods in FractionTest more
comprehensible –
I would think that adding new methods could be more controversial than
adding
code blocks.

Besides, should anyone in the future wish to extract these code sections
into
separate methods, I doubt that the code blocks would be a hindrance – if
anything,
I imagine that they would it easier, because with the code blocks, it is
a lot
clearer WHAT can be extracted to a method in the first place than
without them.

 > (1) It is helpful to add a // comment for each block, otherwise, it
feels
 > anonymous and weird to me.

I am not sure how adding a comment to the code blocks would be helpful in
this case. The blocks only serve to separate the test cases, and most of
the time,
these test cases differ only in the values that they use, and not in
some defining
characteristic. For example, the first three blocks in testReciprocal()
only test
some different arbitrary fractions, but are otherwise completely
analogous, so I
couldn't think of any other comment than something like "test case 1",
"test case 2",
etc. Granted, the fourth block tests failure with a zero-denominator, so
in this
case, I can understand your point about adding comments. By the way,
some of these
test cases were already commented before I edited the class.

On 6/12/19 3:08 PM, Gary Gregory wrote:

> I've used code blocks in this style in the past but...
>
> (1) It is helpful to add a // comment for each block, otherwise, it feels
> anonymous and weird to me.
> (2) Why not refactor and pull-out methods? This then forces you to _name_
> the methods, instead of the above (anonymous blocks vs. commented blocks.)
>
> Gary
>
> On Wed, Jun 12, 2019 at 9:00 AM Heinrich Bohne <[hidden email]>
> wrote:
>
>> I have been asked to request some feedback on this pull request:
>> https://github.com/apache/commons-numbers/pull/36– specifically, about
>> the introduction of code blocks in the commit "NUMBERS-100: Reduce scope
>> of local variables".
>>
>> I had the idea with the code blocks when I wanted to add a test to the
>> method testAdd() but was intimidated by the huge wall of code contained
>> in the method. When taking a closer look, this code wall is actually
>> composed of several test cases that are completely independent of each
>> other, but because the local variables live throughout the whole method
>> and are re-used in almost every test case, this is not obvious. The more
>> variables are involved, the closer you have to look to figure out which
>> sections are independent of the rest.
>>
>> I think that, with the code blocks, it is instantly obvious that a
>> specific section does not depend on anything that happened before it, or
>> that it does not affect anything that comes after it. So I think that
>> they are preferable to the previous version of the file.
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: [numbers] Code blocks in test methods

Gilles Sadowski-2
Hi.

Le jeu. 13 juin 2019 à 01:34, Heinrich Bohne <[hidden email]> a écrit :

>
>  > (2) Why not refactor and pull-out methods? This then forces you to _name_
>  > the methods, instead of the above (anonymous blocks vs. commented
> blocks.)
>
> I did not pull out the code sections into separate methods because I had no
> intention of re-structuring the whole class. I only wanted to fix a bug in
> the class Fraction and add a test case in FractionTest that would have
> failed
> due to this bug, and in theprocess organize that which was already
> present in
> FractionTest a bit better, because it has been pointed out to me that new
> contributions should not only strive to improve functionality but also
> readability.
> Introducing those code blocks seemed like a straightforward way of
> making the
> mess in the bodies of some of the test methods in FractionTest more
> comprehensible –
> I would think that adding new methods could be more controversial than
> adding
> code blocks.
>
> Besides, should anyone in the future wish to extract these code sections
> into
> separate methods, I doubt that the code blocks would be a hindrance – if
> anything,
> I imagine that they would it easier, because with the code blocks, it is
> a lot
> clearer WHAT can be extracted to a method in the first place than
> without them.
>
>  > (1) It is helpful to add a // comment for each block, otherwise, it
> feels
>  > anonymous and weird to me.
>
> I am not sure how adding a comment to the code blocks would be helpful in
> this case. The blocks only serve to separate the test cases, and most of
> the time,
> these test cases differ only in the values that they use, and not in
> some defining
> characteristic. For example, the first three blocks in testReciprocal()
> only test
> some different arbitrary fractions, but are otherwise completely
> analogous, so I
> couldn't think of any other comment than something like "test case 1",
> "test case 2",
> etc. Granted, the fourth block tests failure with a zero-denominator, so
> in this
> case, I can understand your point about adding comments. By the way,
> some of these
> test cases were already commented before I edited the class.

Overall, readability is not worse; and the addition of blocks is in
the spirit of "small steps".
My first reaction was also that functions would be more readable,
but some would be quite trivial, adding another layer for not much
improvement.
Anyway, this can be done in another pass (one is already foreseen
in order to switch to Junit5).  So, any objection to merging the PR?

Thanks,
Gilles

>
> On 6/12/19 3:08 PM, Gary Gregory wrote:
>
> > I've used code blocks in this style in the past but...
> >
> > (1) It is helpful to add a // comment for each block, otherwise, it feels
> > anonymous and weird to me.
> > (2) Why not refactor and pull-out methods? This then forces you to _name_
> > the methods, instead of the above (anonymous blocks vs. commented blocks.)
> >
> > Gary
> >
> > On Wed, Jun 12, 2019 at 9:00 AM Heinrich Bohne <[hidden email]>
> > wrote:
> >
> >> I have been asked to request some feedback on this pull request:
> >> https://github.com/apache/commons-numbers/pull/36– specifically, about
> >> the introduction of code blocks in the commit "NUMBERS-100: Reduce scope
> >> of local variables".
> >>
> >> I had the idea with the code blocks when I wanted to add a test to the
> >> method testAdd() but was intimidated by the huge wall of code contained
> >> in the method. When taking a closer look, this code wall is actually
> >> composed of several test cases that are completely independent of each
> >> other, but because the local variables live throughout the whole method
> >> and are re-used in almost every test case, this is not obvious. The more
> >> variables are involved, the closer you have to look to figure out which
> >> sections are independent of the rest.
> >>
> >> I think that, with the code blocks, it is instantly obvious that a
> >> specific section does not depend on anything that happened before it, or
> >> that it does not affect anything that comes after it. So I think that
> >> they are preferable to the previous version of the file.
> >>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [numbers] Code blocks in test methods

Eric Barnhill
I agree this increases readability and is nice.+1

The only thing that gives me the creeps is the force push in the PR. But
that is off topic, so another email for that.

On Thu, Jun 13, 2019 at 5:42 AM Gilles Sadowski <[hidden email]>
wrote:

> Hi.
>
> Le jeu. 13 juin 2019 à 01:34, Heinrich Bohne <[hidden email]> a
> écrit :
> >
> >  > (2) Why not refactor and pull-out methods? This then forces you to
> _name_
> >  > the methods, instead of the above (anonymous blocks vs. commented
> > blocks.)
> >
> > I did not pull out the code sections into separate methods because I had
> no
> > intention of re-structuring the whole class. I only wanted to fix a bug
> in
> > the class Fraction and add a test case in FractionTest that would have
> > failed
> > due to this bug, and in theprocess organize that which was already
> > present in
> > FractionTest a bit better, because it has been pointed out to me that new
> > contributions should not only strive to improve functionality but also
> > readability.
> > Introducing those code blocks seemed like a straightforward way of
> > making the
> > mess in the bodies of some of the test methods in FractionTest more
> > comprehensible –
> > I would think that adding new methods could be more controversial than
> > adding
> > code blocks.
> >
> > Besides, should anyone in the future wish to extract these code sections
> > into
> > separate methods, I doubt that the code blocks would be a hindrance – if
> > anything,
> > I imagine that they would it easier, because with the code blocks, it is
> > a lot
> > clearer WHAT can be extracted to a method in the first place than
> > without them.
> >
> >  > (1) It is helpful to add a // comment for each block, otherwise, it
> > feels
> >  > anonymous and weird to me.
> >
> > I am not sure how adding a comment to the code blocks would be helpful in
> > this case. The blocks only serve to separate the test cases, and most of
> > the time,
> > these test cases differ only in the values that they use, and not in
> > some defining
> > characteristic. For example, the first three blocks in testReciprocal()
> > only test
> > some different arbitrary fractions, but are otherwise completely
> > analogous, so I
> > couldn't think of any other comment than something like "test case 1",
> > "test case 2",
> > etc. Granted, the fourth block tests failure with a zero-denominator, so
> > in this
> > case, I can understand your point about adding comments. By the way,
> > some of these
> > test cases were already commented before I edited the class.
>
> Overall, readability is not worse; and the addition of blocks is in
> the spirit of "small steps".
> My first reaction was also that functions would be more readable,
> but some would be quite trivial, adding another layer for not much
> improvement.
> Anyway, this can be done in another pass (one is already foreseen
> in order to switch to Junit5).  So, any objection to merging the PR?
>
> Thanks,
> Gilles
>
> >
> > On 6/12/19 3:08 PM, Gary Gregory wrote:
> >
> > > I've used code blocks in this style in the past but...
> > >
> > > (1) It is helpful to add a // comment for each block, otherwise, it
> feels
> > > anonymous and weird to me.
> > > (2) Why not refactor and pull-out methods? This then forces you to
> _name_
> > > the methods, instead of the above (anonymous blocks vs. commented
> blocks.)
> > >
> > > Gary
> > >
> > > On Wed, Jun 12, 2019 at 9:00 AM Heinrich Bohne <[hidden email]>
> > > wrote:
> > >
> > >> I have been asked to request some feedback on this pull request:
> > >> https://github.com/apache/commons-numbers/pull/36– specifically,
> about
> > >> the introduction of code blocks in the commit "NUMBERS-100: Reduce
> scope
> > >> of local variables".
> > >>
> > >> I had the idea with the code blocks when I wanted to add a test to the
> > >> method testAdd() but was intimidated by the huge wall of code
> contained
> > >> in the method. When taking a closer look, this code wall is actually
> > >> composed of several test cases that are completely independent of each
> > >> other, but because the local variables live throughout the whole
> method
> > >> and are re-used in almost every test case, this is not obvious. The
> more
> > >> variables are involved, the closer you have to look to figure out
> which
> > >> sections are independent of the rest.
> > >>
> > >> I think that, with the code blocks, it is instantly obvious that a
> > >> specific section does not depend on anything that happened before it,
> or
> > >> that it does not affect anything that comes after it. So I think that
> > >> they are preferable to the previous version of the file.
> > >>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>