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

#594 RqMultipart.java - Removing the warning suppression (PMD.ConstructorOnlyInitializesOrCallOtherConstructors) #641

Merged
merged 4 commits into from
Mar 6, 2016

Conversation

dalifreire
Copy link
Contributor

Issue #594 - Solving puzzle 558-afc1c081 in
src/main/java/org/takes/rq/RqMultipart.java:131-135

  • The class was refactored according to this article: http://www.yegor256.com/2015/05/07/ctors-must-be-code-free.html
  • Removes the warning suppression (PMD.ConstructorOnlyInitializesOrCallOtherConstructors)
  • Create static methods (body(final Request req) and buffer(final Request req)) to the constructor contains only variables initialization and other constructor calls.

Solving puzzle 558-afc1c081 in
src/main/java/org/takes/rq/RqMultipart.java:131-135
@dalifreire
Copy link
Contributor Author

@davvd Could you find someone to review it?

@davvd
Copy link

davvd commented Mar 4, 2016

@dalifreire Thanks, let me find someone who can review this pull request

@davvd
Copy link

davvd commented Mar 4, 2016

@krzyk it's yours,please review

@krzyk
Copy link
Contributor

krzyk commented Mar 4, 2016

@dalifreire please provide a better description of the PR, write what you did

@krzyk
Copy link
Contributor

krzyk commented Mar 4, 2016

@dalifreire same for the PR title, saying that you solved some bug/todo ID is not enough, PR title should be a short description of the work, and the description should be more elaborate information

@dalifreire dalifreire changed the title #594 RqMultipart.java - Puzzle 558-afc1c081 #594 Removing the warning suppression (PMD.ConstructorOnlyInitializesOrCallOtherConstructors) Mar 4, 2016
@dalifreire dalifreire changed the title #594 Removing the warning suppression (PMD.ConstructorOnlyInitializesOrCallOtherConstructors) #594 RqMultipart.java - Removing the warning suppression (PMD.ConstructorOnlyInitializesOrCallOtherConstructors) Mar 4, 2016
@dalifreire
Copy link
Contributor Author

@krzyk Please, could you take a look again?

// @checkstyle MagicNumberCheck (1 line)
Math.min(8192, stream.available())
);
this.body = Base.body(req);
Copy link
Contributor

Choose a reason for hiding this comment

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

@dalifreire making a static method is a workaround not a solution. Here you could store the stream as a field and create Channel in requests method and pass the created variable to copy and make.

@krzyk
Copy link
Contributor

krzyk commented Mar 4, 2016

@dalifreire added one more comment

@dalifreire
Copy link
Contributor Author

@krzyk Thanks for review! Please, take a look if it's better now...

* @throws IOException If fails
* @checkstyle ExecutableStatementCountCheck (2 lines)
*/
private void copy(final WritableByteChannel target,
final byte[] boundary) throws IOException {
final byte[] boundary,
final ReadableByteChannel body) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

@dalifreire you can put body in the previous line, and leave only throws in this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@krzyk Done! :)

@krzyk
Copy link
Contributor

krzyk commented Mar 5, 2016

@dalifreire one more comment

@dalifreire
Copy link
Contributor Author

@krzyk Could you review again?

@dalifreire
Copy link
Contributor Author

@krzyk Please, could you take a look if everything is right now?

@krzyk
Copy link
Contributor

krzyk commented Mar 5, 2016

@dalifreire looks good

@krzyk
Copy link
Contributor

krzyk commented Mar 5, 2016

@rultor merge pls

@rultor
Copy link
Collaborator

rultor commented Mar 5, 2016

@rultor merge pls

@krzyk Thanks for your request. @yegor256 Please confirm this.

@yegor256
Copy link
Owner

yegor256 commented Mar 6, 2016

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Mar 6, 2016

@rultor merge

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

@rultor rultor merged commit d1bec6a into yegor256:master Mar 6, 2016
@dalifreire dalifreire deleted the #594-RqMultipart branch March 6, 2016 19:49
@rultor
Copy link
Collaborator

rultor commented Mar 6, 2016

@rultor merge

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

@davvd
Copy link

davvd commented Mar 7, 2016

@elenavolokhova review this ticket please, for compliance with our quality rules

@davvd
Copy link

davvd commented Mar 7, 2016

@rultor deploy now pls

@rultor
Copy link
Collaborator

rultor commented Mar 7, 2016

@rultor deploy now pls

@davvd OK, I'll try to deploy now. You can check the progress here

@rultor
Copy link
Collaborator

rultor commented Mar 7, 2016

@rultor deploy now pls

@davvd Done! FYI, the full log is here (took me 13min)

@elenavolokhova
Copy link

@davvd Looks good!

@davvd
Copy link

davvd commented Mar 8, 2016

@davvd Looks good!

@elenavolokhova thanks a lot :)

@davvd
Copy link

davvd commented Mar 10, 2016

@krzyk just added 10 mins to @elenavolokhova (for QA), payment ID is 79950705; 23 mins sent to your balance (ID 79950750), many thanks! It took 57 hours and 28 mins.; you're getting extra minutes here (c=8); added +23 to your rating, now it is equal to +6129

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

Successfully merging this pull request may close these issues.

6 participants