New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

#532 Declarative fix of AvgOf() bug on Double edge values #581

Merged

rultor merged 15 commits into yegor256:master from rexim:532

Jan 29, 2018

Contributor

rexim commented Jan 26, 2018

#532 is mine now: #532 (comment) Based on #543

Close #532 close #543

allanwsilva and others added 11 commits

January 10, 2018 14:36


          #532 bugfix

753bfeb


          Fixing code formatting issues..

cb8bfa4


          Same mean calculation applied on all Avg ctors to handle max values f…

91c4467

…or all Java Number types (Float.MAX_VALUE, Integer.MAX_VALUE, Long.MAX_VALUE)


          Just added a few more unit tests as @fabriciofx suggested

48bbdda


          Fix as per @svendiedrichsen comment

a607fc9


          refactored AvgOf to prevent precision and overflow errors.

358b7f1


          Merge branch 'master' into 532

ea277e7


          Merge branch 'avg' of https://github.com/svendiedrichsen/cactoos into…

… 532


          AvgOf reviewed and finished for now. Refactored following @svendiedri…

b74508d

…chsen code polish (nice and clean) and added more test cases according @fabriciofx comments.

Decimal precision problem solved as well.


          Reimplement AvgOf in declarative style #532

a00641e


          Add AvgOf test with Scalar compositions #532

fdd3766

Collaborator

0crat commented Jan 26, 2018

@yegor256 /z please, pay attention to this pull request

0crat added the scope label

Collaborator

0crat commented Jan 26, 2018

Job #581 is now in scope, role is REV

codecov-io commented Jan 26, 2018 •

edited

Loading

Codecov Report

Merging #581 into master will increase coverage by 0.46%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #581      +/-   ##
============================================
+ Coverage      67.4%   67.86%   +0.46%     
+ Complexity      994      971      -23     
============================================
  Files           207      207              
  Lines          3436     3277     -159     
  Branches        254      214      -40     
============================================
- Hits           2316     2224      -92     
+ Misses         1049      998      -51     
+ Partials         71       55      -16

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/org/cactoos/scalar/AvgOf.java	`100% <100%> (+35.32%)`	`16 <15> (-25)`	⬇️
src/main/java/org/cactoos/scalar/Ternary.java	`100% <0%> (+5.55%)`	`9% <0%> (+1%)`	⬆️
...c/main/java/org/cactoos/scalar/NumberEnvelope.java	`100% <0%> (+6.25%)`	`10% <0%> (+1%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78950e6...7d9e4f7. Read the comment docs.

Collaborator

0crat commented Jan 26, 2018

This pull request #581 is assigned to @neonailol /z. The budget is 15 minutes, see §4. Please, read §27 and when you decide to accept the changes, inform @yegor256 /z (the architect) right in this ticket. If you decide that this PR should not be accepted ever, also inform the architect.

neonailol suggested changes

View reviewed changes

src/main/java/org/cactoos/scalar/AvgOf.java Outdated

+                              (len) -> len > 0,
+                              (len) -> new Reduced<>(
+                                  BigDecimal.ZERO,
+                                  BigDecimal::add,

Contributor

neonailol Jan 26, 2018

Probably should use BigDecimal#add(java.math.BigDecimal, java.math.MathContext) to be consisted with divide below

src/main/java/org/cactoos/scalar/AvgOf.java Outdated

-                      });
+                      this(
+                          new Mapped<>(
+                              (number) -> () -> number,

Contributor

neonailol Jan 26, 2018

unnecessary parenthises, does not correspond with style used in library

src/main/java/org/cactoos/scalar/AvgOf.java Outdated

-                      });
+                      this(
+                          new Mapped<>(
+                              (number) -> () -> number,

Contributor

neonailol Jan 26, 2018

unnecessary parenthises, does not correspond with style used in library

src/main/java/org/cactoos/scalar/AvgOf.java Outdated

-                      });
+                      this(
+                          new Mapped<>(
+                              (number) -> () -> number,

Contributor

neonailol Jan 26, 2018

unnecessary parenthises, does not correspond with style used in library

src/main/java/org/cactoos/scalar/AvgOf.java Outdated

-                      });
+                      this(
+                          new Mapped<>(
+                              (number) -> () -> number,

Contributor

neonailol Jan 26, 2018

unnecessary parenthises, does not correspond with style used in library

src/main/java/org/cactoos/scalar/AvgOf.java Outdated

+                                  BigDecimal.ZERO,
+                                  BigDecimal::add,
+                                  new Mapped<>(
+                                      (number) -> BigDecimal.valueOf(

Contributor

neonailol Jan 26, 2018

unnecessary parenthises, does not correspond with style used in library

src/main/java/org/cactoos/scalar/AvgOf.java Outdated

+                                  BigDecimal.valueOf(len),
+                                  MathContext.DECIMAL128
+                              ).doubleValue(),
+                              (len) -> 0.0

Contributor

neonailol Jan 26, 2018

unnecessary parenthises, does not correspond with style used in library

src/test/java/org/cactoos/scalar/AvgOfTest.java

+                  }
+                  @Test
+                  public void withIterableOfScalars() {

Contributor

neonailol Jan 26, 2018

in this test type of new IterableOf<Scalar<Number>>(() -> 1L, () -> 2L, () -> 10L) can be inferred, so params are not neccecary

Contributor Author

rexim Jan 26, 2018

Without it the compiler complains:

[WARNING] yegor256/cactoos/src/test/java/org/cactoos/scalar/AvgOfTest.java:[358,17] unchecked generic array creation for varargs parameter of type org.cactoos.Scalar<java.lang.Number>[]

And we have -Werror enabled.

src/test/java/org/cactoos/scalar/AvgOfTest.java Outdated

+                              () -> new MinOf(1.0d, 2.0d),
+                              () -> new MaxOf(2.0d, 4.0d),
+                              () -> new SumOf(1.0d, 2.0d, 2.0d),
+                              new Ternary<>(true, 5.0, 1.0d)

Contributor

neonailol Jan 26, 2018

why just 5.0 without d or why d is need here at all?

Contributor Author

rexim Jan 26, 2018 •

edited

Loading

@neonailol typo. They (d-s) were in the original code I inherited. I kept using the for the consistency. I can remove them if it does not correspond with the style of the library. :)

src/test/java/org/cactoos/scalar/AvgOfTest.java Outdated

+                  public void withFloatScalarsIntValue() {
+                      MatcherAssert.assertThat(
+                          new AvgOf(
+                              () -> 1f, () -> 2f, () -> 10f

Contributor

neonailol Jan 26, 2018

please format floats as 1.0f

rexim added 4 commits

January 27, 2018 02:28


          Remove unnecessary parenthises #532

f07e6e3


          Use BigDecimal#add(java.math.BigDecimal, java.math.MathContext) #532

6dc8535


          Format floats #532


          typo #532

7d9e4f7

Contributor Author

rexim commented Jan 26, 2018

@neonailol fixed your remarks. Please take a look again.

neonailol approved these changes

View reviewed changes

Contributor

neonailol commented Jan 27, 2018

Thank you, looks good to me

Contributor

neonailol commented Jan 27, 2018

@yegor256 this pull request can be merged into master

Owner

yegor256 commented Jan 29, 2018

Collaborator

rultor commented Jan 29, 2018

@rultor merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

rultor merged commit 7d9e4f7 into yegor256:master

Collaborator

rultor commented Jan 29, 2018

@rultor merge

@yegor256 Done! FYI, the full log is here (took me 12min)

Collaborator

0crat commented Jan 29, 2018

Order was successfully finished: +15 points just awarded to @neonailol /z, total is +180

Collaborator

0crat commented Jan 29, 2018

Payment to ARC for a closed pull request, as in §28: +15 points just awarded to @yegor256 /z, total is +8755

rexim deleted the 532 branch

January 29, 2018 13:23

Collaborator

0crat commented Jan 29, 2018

The job #581 is now out of scope

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels