#113 RsWithHeaders must reuse RsWithHeader #210

dmzaytsev · 2015-04-25T07:38:04Z

For #113

Got rid code duplication in RsWithHeader and RsWithHeaders according the task description

davvd · 2015-04-25T10:02:51Z

@dmzaytsev Thanks, someone will review your pull request soon

davvd · 2015-04-25T10:23:47Z

@pinaf it's yours,please review

pinaf · 2015-04-25T16:48:22Z

src/test/java/org/takes/rs/RsWithHeadersTest.java

@@ -47,8 +48,8 @@ public void addsHeadersToResponse() throws IOException {
            new RsPrint(
                new RsWithHeaders(
                    new RsEmpty(),
-                    "Host: www.example.com ",
-                    "Content-Type: text/xml "
+                    "Host: www.example.com",


@dmzaytsev let's extract this into a private final String variable.

pinaf · 2015-04-25T16:49:08Z

@dmzaytsev nice :) 3 minor comments above.

dmzaytsev · 2015-04-25T16:56:11Z

@pinaf done :)

pinaf · 2015-04-25T16:58:25Z

@dmzaytsev thanks. this design is very "russian doll" style.

dmzaytsev · 2015-04-25T17:07:04Z

@pinaf how would you solved this problem?

pinaf · 2015-04-25T17:07:38Z

@rultor merge

rultor · 2015-04-25T17:08:04Z

@rultor merge

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

pinaf · 2015-04-25T17:10:06Z

@dmzaytsev I wouldn't reuse the RqWithHeader class. Instead I'd build the headers once in the constructor as a list and just return it through the head method. It would be more efficient and, since the code is so simple, no readability would be lost.

dmzaytsev · 2015-04-25T17:10:53Z

@pinaf build failed .. weird
I had no errors before push

dmzaytsev · 2015-04-25T17:15:42Z

@pinaf use of the RqWithHeader was a condition of the task
anyway thanks for review

pinaf · 2015-04-25T17:16:01Z

@dmzaytsev only with java 8 on travis. seems like a network issue.
yes, you did the task as requested :)

dmzaytsev · 2015-04-28T03:30:56Z

@yegor256 take a look this PR #210 please.
Thank you

pinaf · 2015-04-28T15:46:42Z

@rultor merge

rultor · 2015-04-28T17:11:25Z

@rultor merge

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

dmzaytsev · 2015-04-30T05:17:52Z

@yegor256 this PR needs to be merged
We are waiting 5 days already.
Please pay your attention.

dmzaytsev · 2015-05-04T06:58:53Z

@yegor256 take a look this PR please

yegor256 · 2015-05-04T23:49:10Z

@dmzaytsev this is a wrong design. I disagree with @pinaf here and even wrote an article about it. It will be published in a few days, but you can check the draft here: https://raw.githubusercontent.com/yegor256/blog/master/_drafts/ctors.md

Constructors should be light and never (ideally) do anything except assignments.

pinaf · 2015-05-05T00:52:54Z

@yegor256 what do you mean exactly? I pointed out an opinion but his design was different.

pinaf · 2015-05-05T00:54:39Z

@yegor256 full composition with one object instance per element in iterable?

yegor256 · 2015-05-05T00:54:46Z

@pinaf I'm talking about this comment you made: #210 (comment)

pinaf · 2015-05-05T00:59:35Z

@yegor256 yeah, I see - I agree with the article. However, you do note he chose the compositional style - kind of - instead of going with my opinion.

yegor256 · 2015-05-05T01:03:08Z

@pinaf current implementation using RsWithHeader correctly, making many instances of this class. but this shouldn't happen in ctor, but in actual head() call instead

pinaf · 2015-05-05T01:05:09Z

@yegor256 ok - so the missing part is the delaying of the iteration, like you point out in the article. thanks!

dmzaytsev · 2015-05-05T11:19:32Z

@yegor256 useful article, thanks!
the logic has been moved to the head() call.
looks good now ?

yegor256 · 2015-05-05T18:28:40Z

src/main/java/org/takes/rs/RsWithHeaders.java

+        for (final CharSequence hdr: headers) {
+            resp = new RsWithHeader(resp, hdr);
+        }
+        final List<String> ret = new ArrayList<String>(1);


I didn't get this... why can't you just do return resp.head()?

yegor256 · 2015-05-05T18:29:03Z

@dmzaytsev yes, better, but you over-engineered it, I believe. see my comment

dmzaytsev · 2015-05-05T18:55:40Z

@yegor256 you're right
take a look please

yegor256 · 2015-05-05T19:09:46Z

@rultor try to merge

rultor · 2015-05-05T19:10:00Z

@rultor try to merge

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

rultor · 2015-05-05T19:22:45Z

@rultor try to merge

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

davvd · 2015-05-07T11:07:26Z

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

yegor256 · 2015-05-08T01:51:36Z

@dmzaytsev @pinaf here is the article published: http://www.yegor256.com/2015/05/07/ctors-must-be-code-free.html

elenavolokhova · 2015-05-08T05:39:55Z

@davvd Looks good!

davvd · 2015-05-08T09:08:26Z

@davvd Looks good!

@elenavolokhova thanks for the review and a good remark

davvd · 2015-05-09T11:03:57Z

@pinaf I added 10 mins to @elenavolokhova (for QA review) in transaction 56872197... Thanks so much! Your account was topped up for 25 mins (transaction ID is AP-2NS64077DN239632Y, the task took 264 hours and 25 mins)... you're getting extra minutes here (c=10)... +25 added to your rating, current score is: +7693

davvd · 2015-05-09T11:16:32Z

@rultor deploy now pls

rultor · 2015-05-09T11:28:21Z

@rultor deploy now pls

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

rultor · 2015-05-09T11:39:02Z

@rultor deploy now pls

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

yegor256#113 refactored RsWithHeaders

07a1ecd

dmzaytsev mentioned this pull request Apr 25, 2015

RsWithHeaders must reuse RsWithHeader #113

Closed

davvd added CR @pinaf labels Apr 25, 2015

pinaf reviewed Apr 25, 2015
View reviewed changes

yegor256#113 got rid the checkstyle suppression

35b48e9

dmzaytsev added 2 commits May 5, 2015 15:45

Merge remote-tracking branch 'upstream/master' into 113

aba000b

removed the logic from the ctor

2d85d15

yegor256 reviewed May 5, 2015
View reviewed changes

simplified

690d603

qulice fix

fe57e6a

rultor merged commit fe57e6a into yegor256:master May 5, 2015

super132 mentioned this pull request May 6, 2015

broken test org.takes.rq.RqMultipartTest.consumesHttpRequest #253

Closed

#113 RsWithHeaders must reuse RsWithHeader #210

#113 RsWithHeaders must reuse RsWithHeader #210

Conversation

dmzaytsev commented Apr 25, 2015

davvd commented Apr 25, 2015

davvd commented Apr 25, 2015

pinaf Apr 25, 2015

Choose a reason for hiding this comment

pinaf commented Apr 25, 2015

dmzaytsev commented Apr 25, 2015

pinaf commented Apr 25, 2015

dmzaytsev commented Apr 25, 2015

pinaf commented Apr 25, 2015

rultor commented Apr 25, 2015

pinaf commented Apr 25, 2015

dmzaytsev commented Apr 25, 2015

dmzaytsev commented Apr 25, 2015

pinaf commented Apr 25, 2015

dmzaytsev commented Apr 28, 2015

pinaf commented Apr 28, 2015

rultor commented Apr 28, 2015

dmzaytsev commented Apr 30, 2015

dmzaytsev commented May 4, 2015

yegor256 commented May 4, 2015

pinaf commented May 5, 2015

pinaf commented May 5, 2015

yegor256 commented May 5, 2015

pinaf commented May 5, 2015

yegor256 commented May 5, 2015

pinaf commented May 5, 2015

dmzaytsev commented May 5, 2015

yegor256 May 5, 2015

Choose a reason for hiding this comment

yegor256 commented May 5, 2015

dmzaytsev commented May 5, 2015

yegor256 commented May 5, 2015

rultor commented May 5, 2015

rultor commented May 5, 2015

davvd commented May 7, 2015

yegor256 commented May 8, 2015

elenavolokhova commented May 8, 2015

davvd commented May 8, 2015

davvd commented May 9, 2015

davvd commented May 9, 2015

rultor commented May 9, 2015

rultor commented May 9, 2015