Hi. Le jeu. 13 juin 2019 à 01:34, Heinrich Bohne <heinrich.bo...@gmx.at> 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 <heinrich.bo...@gmx.at> > > 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: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
