#666: Refactored the class RqMtBase to reduce the coupling #668

essobedo · 2016-04-23T16:41:03Z

Fix for #666

The goal of this PR is to refactor the class RqMtBase in order to reduce the overall coupling pf this class

davvd · 2016-04-23T18:17:36Z

@essobedo Thanks, I will find someone to review this PR soon

davvd · 2016-04-23T18:23:27Z

@dmzaytsev review this one,please

dmzaytsev · 2016-04-24T15:29:59Z

src/test/java/org/takes/rq/RqTempTest.java

+            RqTempTest.class.getName(),
+            ".tmp"
+        );
+        final BufferedWriter out = new BufferedWriter(new FileWriter(file));


@essobedo I think it can be implemented shortly as Takes moved to java 7

Files.write( file.toPath(), Arrays.asList("Temp file deletion test"), StandardCharsets.UTF_8 );

dmzaytsev · 2016-04-24T15:31:04Z

@essobedo please see my comment

…s of Java 7

essobedo · 2016-04-24T15:46:03Z

@dmzaytsev done, thx for the feedback

dmzaytsev · 2016-04-24T15:54:38Z

@essobedo thanks

dmzaytsev · 2016-04-24T15:54:46Z

@rultor merge

rultor · 2016-04-24T15:55:03Z

@rultor merge

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

essobedo · 2016-04-25T13:14:44Z

@yegor256 please check

essobedo · 2016-04-26T11:56:00Z

@yegor256 ping

essobedo · 2016-04-27T11:12:27Z

@yegor256 Please confirm

essobedo · 2016-04-28T08:25:16Z

@yegor256 please

yegor256 · 2016-04-28T14:11:29Z

src/main/java/org/takes/rq/multipart/RqMtBase.java

@@ -140,15 +129,15 @@ public RqMtBase(final Request req) throws IOException {
        if (values == null) {
            iter = new VerboseIterable<>(
                Collections.<Request>emptyList(),
-                new Sprintf(
+                String.format(


@essobedo what is the reason for this change?

@yegor256 I needed to reduce the coupling, and as I tested the result of both and got the same result, I believed that I could replace it. Why is it not correct? What did I miss? What does Sprintf that String.format doesn't do?

@essobedo String.format() will print the string right here, while Sprintf will print only when it's needed. in most cases - never.

yegor256 · 2016-04-28T14:11:47Z

@essobedo some questions from me above

essobedo · 2016-04-28T15:17:21Z

@yegor256 I provided the answer, please check it

essobedo · 2016-04-29T11:05:27Z

@yegor256 please check my answers

essobedo · 2016-05-01T09:23:29Z

@yegor256 please check my answer

essobedo · 2016-05-03T10:48:10Z

@yegor256 ping

yegor256 · 2016-05-03T10:52:47Z

src/main/java/org/takes/rq/RqLive.java

@@ -45,7 +45,7 @@
 * @since 0.1
 */
 @EqualsAndHashCode(callSuper = true)
-public final class RqLive extends RqWrap {
+public class RqLive extends RqWrap {


@essobedo check this: http://www.yegor256.com/2014/11/20/seven-virtues-of-good-object.html#7-his-class-is-either-final-or-abstract

yegor256 · 2016-05-03T10:54:27Z

@essobedo please excuse me for the delay. one more comment above

essobedo · 2016-05-03T12:14:45Z

@yegor256 fixed, please check again

yegor256 · 2016-05-03T13:13:22Z

src/main/java/org/takes/rq/RqTemp.java

+    /**
+     * Creates a {@code RqTemp} with the specified temporary file.
+     * @param file The temporary that will be automatically deleted when the
+     *  body of the request will be closed.


@essobedo I don't see where this file is actually deleted... do I miss something?

@essobedo ignore me

yegor256 · 2016-05-03T13:14:50Z

@essobedo see my concern above

essobedo · 2016-05-03T15:54:09Z

@yegor256 RqTemp is no more public and has been moved into the multipart package, please check again

yegor256 · 2016-05-03T19:13:42Z

@rultor try to merge

rultor · 2016-05-03T19:14:41Z

@rultor try to merge

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

rultor · 2016-05-03T19:30:54Z

@rultor try to merge

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

davvd · 2016-05-04T20:36:43Z

@elenavolokhova please, check this issue for QA compliance, as per par.24

davvd · 2016-05-04T20:38:09Z

@rultor please deploy

rultor · 2016-05-04T20:38:57Z

@rultor please deploy

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

rultor · 2016-05-04T20:56:07Z

@rultor please deploy

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

elenavolokhova · 2016-05-04T22:08:43Z

@dmzaytsev
According to our quality rules:

The code reviewer found at least three problems in the code.

Only one issue was found during review.
Please confirm that you will try to find at least three problems next time.

dmzaytsev · 2016-05-06T10:54:44Z

@elenavolokhova I confirm

elenavolokhova · 2016-05-06T14:09:25Z

@davvd Quality is acceptable here.

davvd · 2016-05-06T15:23:57Z

@davvd Quality is acceptable here.

@elenavolokhova thank you, let's try to make it "good" next time

davvd · 2016-05-06T15:30:54Z

@dmzaytsev added 10 mins to @elenavolokhova, for QA review, payment code is 85775163

thanks, paid, 20 mins to your account, payment ID is 85775198, task took 241 hours and 33 mins to complete

you're getting extra minutes here (c=5)

+20 added to your rating, at the moment it is: +1304

essobedo added 2 commits April 23, 2016 16:37

#666: Refactored the class RqMtBase to reduce the coupling

7dc62db

#666: Fix a typo in the javadoc of RqTemp

47a1761

essobedo mentioned this pull request Apr 23, 2016

RqMtBase.java:80-83: This class is highly coupled,... #666

Closed

davvd added CR @dmzaytsev 15 mins labels Apr 23, 2016

dmzaytsev reviewed Apr 24, 2016
View reviewed changes

#666: Improved the test RqTempTest.deletesTempFile() by using feature…

c69b575

…s of Java 7

yegor256 reviewed Apr 28, 2016
View reviewed changes

yegor256 reviewed May 3, 2016
View reviewed changes

essobedo added 2 commits May 3, 2016 11:19

Merge branch 'master' of https://github.com/yegor256/takes into 666

6d604ca

#666: Put Sprintf back and got rid of RqWithHeader

73543d3

yegor256 reviewed May 3, 2016
View reviewed changes

essobedo added 2 commits May 3, 2016 15:12

#666: Improved the javadoc and fixed the conflict

ed2e1d3

#666: Moved RqTemp into the package multipart

86e34fd

rultor merged commit 86e34fd into yegor256:master May 3, 2016

essobedo deleted the 666 branch May 3, 2016 20:10

#666: Refactored the class RqMtBase to reduce the coupling #668

#666: Refactored the class RqMtBase to reduce the coupling #668

Conversation

essobedo commented Apr 23, 2016

davvd commented Apr 23, 2016

davvd commented Apr 23, 2016

dmzaytsev Apr 24, 2016 • edited Loading

Choose a reason for hiding this comment

dmzaytsev commented Apr 24, 2016

essobedo commented Apr 24, 2016

dmzaytsev commented Apr 24, 2016

dmzaytsev commented Apr 24, 2016

rultor commented Apr 24, 2016

essobedo commented Apr 25, 2016

essobedo commented Apr 26, 2016

essobedo commented Apr 27, 2016

essobedo commented Apr 28, 2016

yegor256 Apr 28, 2016

Choose a reason for hiding this comment

essobedo Apr 28, 2016

Choose a reason for hiding this comment

yegor256 May 3, 2016

Choose a reason for hiding this comment

yegor256 commented Apr 28, 2016

essobedo commented Apr 28, 2016

essobedo commented Apr 29, 2016

essobedo commented May 1, 2016

essobedo commented May 3, 2016

yegor256 May 3, 2016

Choose a reason for hiding this comment

yegor256 commented May 3, 2016

essobedo commented May 3, 2016

yegor256 May 3, 2016

Choose a reason for hiding this comment

yegor256 May 3, 2016

Choose a reason for hiding this comment

yegor256 commented May 3, 2016

essobedo commented May 3, 2016

yegor256 commented May 3, 2016

rultor commented May 3, 2016

rultor commented May 3, 2016

davvd commented May 4, 2016

davvd commented May 4, 2016

rultor commented May 4, 2016

rultor commented May 4, 2016

elenavolokhova commented May 4, 2016

dmzaytsev commented May 6, 2016

elenavolokhova commented May 6, 2016

davvd commented May 6, 2016

davvd commented May 6, 2016

dmzaytsev Apr 24, 2016 •

edited

Loading