Skip to content
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.

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

#308: Equality #309

Closed
wants to merge 4 commits into from
Closed

#308: Equality #309

wants to merge 4 commits into from

Conversation

fabriciofx
Copy link
Contributor

As per #308.

@codecov-io
Copy link

codecov-io commented Jul 11, 2017

Codecov Report

Merging #309 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #309      +/-   ##
============================================
+ Coverage     77.76%   77.84%   +0.08%     
- Complexity      431      437       +6     
============================================
  Files           137      138       +1     
  Lines          1385     1404      +19     
  Branches         68       72       +4     
============================================
+ Hits           1077     1093      +16     
- Misses          288      291       +3     
  Partials         20       20
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/cactoos/func/Equality.java 100% <100%> (ø) 6 <6> (?)
src/main/java/org/cactoos/func/RetryFunc.java 40% <0%> (-15%) 3% <0%> (ø)

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 766b416...262f784. Read the comment docs.

break;
}
}
return (int) Math.signum(result);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabriciofx please, check the article on the blog, the code is changed there. Pay attention to this thread of comments: http://www.yegor256.com/2017/07/11/how-to-redesign-equals.html#comment-3420679770

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yegor256 Right. I made some changes. Could you check them?

* @since 0.12
* @checkstyle JavadocMethodCheck (500 lines)
*/
public final class EqualityTest {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabriciofx you may be interested in more tests, as we did in this thread: http://www.yegor256.com/2017/07/11/how-to-redesign-equals.html#comment-3420679770

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yegor256 Right. I made some changes. Could you check them?

@yegor256
Copy link
Owner

@fabriciofx thanks for the code, looks good, but please pay attention to my comments above

@llorllale
Copy link
Contributor

@0crat in

@0crat 0crat added the scope label Apr 17, 2018
@0crat
Copy link
Collaborator

0crat commented Apr 17, 2018

@0crat in (here)

@llorllale Job #309 is now in scope, role is REV

@0crat
Copy link
Collaborator

0crat commented Apr 17, 2018

This pull request #309 is assigned to @neonailol/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @llorllale/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer

@0crat
Copy link
Collaborator

0crat commented Apr 22, 2018

@neonailol/z this job was assigned to you 5days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please.

@llorllale
Copy link
Contributor

@0crat resign @neonailol

@0crat
Copy link
Collaborator

0crat commented Apr 25, 2018

@0crat resign @neonailol (here)

@llorllale The user @neonailol/z resigned from #309, please stop working. Reason for job resignation: Order was cancelled

@0crat
Copy link
Collaborator

0crat commented Apr 25, 2018

This pull request #309 is assigned to @SharplEr/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @llorllale/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer

int result = 0;
for (int idx = max; idx > 0; --idx) {
byte first = 0;
if (idx <= left.length) {
Copy link
Contributor

@SharplEr SharplEr Apr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabriciofx Looks like code can be more optimal. You can remove if statements from the loop. Get a longer array and check its last bytes is zero. After that, you can compare a shorter array with remaining part of the longer one with only one if statement.

* @checkstyle JavadocMethodCheck (500 lines)
*/
@SuppressWarnings("PMD.AvoidDuplicateLiterals")
public final class EqualityTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabriciofx You have lost one behavior. When one array longer than another, but have zeros on the tail. Like {1, 1, 0, 0} and {1,1}. As I understand it is an important part of API.

@SharplEr
Copy link
Contributor

@fabriciofx Please pay attention to my remarks.

@SharplEr
Copy link
Contributor

SharplEr commented May 2, 2018

@fabriciofx ping)

@0crat
Copy link
Collaborator

0crat commented May 2, 2018

@SharplEr/z this job was assigned to you 7days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please.

@Vatavuk
Copy link
Contributor

Vatavuk commented May 3, 2018

@llorllale @SharplEr Fabricio is inactive. This job is assigned to me and I've created new PR #809

@0crat
Copy link
Collaborator

0crat commented May 5, 2018

The user @SharplEr/z resigned from #309, please stop working. Reason for job resignation: It is older than 10 days, see §8

@0crat
Copy link
Collaborator

0crat commented May 5, 2018

Resigned on delay, see §8: -15 point(s) just awarded to @SharplEr/z

@0crat
Copy link
Collaborator

0crat commented May 5, 2018

This pull request #309 is assigned to @proshin-roman/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @llorllale/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer

@proshin-roman
Copy link
Contributor

@llorllale This pull request must be closed as there is another one #809 that is reviewed and waiting for merging.

@llorllale
Copy link
Contributor

@proshin-roman thanks

@llorllale llorllale closed this May 5, 2018
@0crat
Copy link
Collaborator

0crat commented May 5, 2018

@elenavolokhova/z please review this job completed by @proshin-roman/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed

@0crat 0crat removed the scope label May 5, 2018
@0crat
Copy link
Collaborator

0crat commented May 5, 2018

The job #309 is now out of scope

@0crat
Copy link
Collaborator

0crat commented May 5, 2018

Payment to ARC for a closed pull request, as in §28: +10 point(s) just awarded to @llorllale/z

@elenavolokhova
Copy link

@0crat quality acceptable

@0crat
Copy link
Collaborator

0crat commented May 6, 2018

Order was finished, quality is "acceptable": +15 point(s) just awarded to @proshin-roman/z

@0crat
Copy link
Collaborator

0crat commented May 6, 2018

Quality review completed: +8 point(s) just awarded to @elenavolokhova/z

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants