#473 Refactoring Request-Line validation #484

xupyprmv · 2015-12-26T11:27:12Z

Refactoring for #473 RqHref.java:83-86: RqMethod already validates Request-Line and... #473

I created RqRequestLine interface that 2 methods:

String requestLineHeader() throws IOException; // returns valid Request-line

String requestLineHeaderToken(Token token) throws IOException; // returns specified token from Request-Line

Also I added class that implements RqRequestLine interface and provides Request-Line validation.

…t-Line and... yegor256#473`

xupyprmv · 2015-12-26T11:54:05Z

@davvd Please proceed with code review.

davvd · 2015-12-26T12:52:01Z

@xupyprmv Many thanks, I will find someone to review it before we merge

davvd · 2015-12-26T13:14:09Z

@dmzaytsev review this one, please

dmzaytsev · 2015-12-28T12:21:23Z

@xupyprmv pull request description explains the solution proposed and contains a link to the original ticket it is related to
the solution should be described more detail

dmzaytsev · 2015-12-28T12:22:49Z

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

+ *
+ * @author Vladimir Maksimenko (xupypr@xupypr.com)
+ * @version $Id$
+ * @since 1.0


@xupyprmv 0.30

@dmzaytsev I take version 1.0 from pom.xml file. Please, can you explain where I can get information about next version. That helps me not to do the same mistake in future.

@xupyprmv I add 0.0.1 to the last release usually
https://github.com/yegor256/takes/releases

@xupyprmv so it will be 0.29.1

dmzaytsev · 2015-12-28T12:41:45Z

@xupyprmv please see a few comments above
also pay attention the indentation step should be 4 spaces

xupyprmv · 2015-12-28T17:41:15Z

@dmzaytsev Thank you for CR. I refactored code according to your remarks. Please, proceed with CR.

dmzaytsev · 2015-12-28T20:26:00Z

src/main/java/org/takes/http/BkReuse.java

- * @author Piotr Pradzynski (prondzyn@gmail.com)
- * @version $Id$
- */
-public final class BkReuse extends BkWrap {


@xupyprmv why did you remove this class?

dmzaytsev · 2015-12-28T20:29:01Z

@xupyprmv just two questions above

xupyprmv · 2015-12-28T20:40:43Z

@dmzaytsev I rollback removing of BkReuse and BkReuseTest. It was my fail. It was a part of work for #456, not for current issue.

dmzaytsev · 2015-12-28T20:47:06Z

@xupyprmv no problem
thanks for PR

dmzaytsev · 2015-12-28T20:47:12Z

@rultor merge

rultor · 2015-12-28T20:48:34Z

@rultor merge

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

xupyprmv · 2016-01-04T07:35:59Z

@yegor256 I refactored code according to your remark. Please, proceed with CR. (Sorry, I'm not wait for end of your previous CR, @davvd urges me to close #473 in 2 days)

yegor256 · 2016-01-04T16:08:27Z

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

+        /**
+         * HTTP Request-line pattern.
+         * [!-~] is for method or extension-method token (octets 33 - 126).
+         * @see <a href="http://www


@xupyprmv this won't work with JavaDoc. just put it all in one long line, checkstyle won't complain

yegor256 · 2016-01-04T16:12:57Z

@xupyprmv the code is good, but your naming is rather messy. I posted some comments above. first rule: use single words for method and variable names. second rule: read this

xupyprmv · 2016-01-04T17:32:08Z

@yegor256 Thanks for advises. I refactored code according to your review. Please proceed with CR.

yegor256 · 2016-01-04T19:54:16Z

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

+        public String header() throws IOException {
+            final String line = this.line();
+            this.matcher(line);
+            return line;


@xupyprmv read this one again: http://www.yegor256.com/2015/12/08/temporal-coupling-between-method-calls.html Your code here should look like this:

public String header() { return this.validated(this.line()); }

See the idea?

@xupyprmv maybe I'm wrong with this exact example, but this.matcher(line) is a code smell. You should never use functions as procedures, ignoring their returns.

yegor256 · 2016-01-04T19:56:05Z

@xupyprmv see my comments above, I don't like the design of private methods. they are very "procedural"

xupyprmv · 2016-01-04T21:33:11Z

@yegor256 I refactored code. It is more "functional" now. The reason why I used "procedural" approach before is to avoid "copy-paste style". Because I need Matcher for two purposes: to validate Request-Line and to extract groups. But for performance reasons I try to use match only once for each call. Please, proceed with CR.

yegor256 · 2016-01-06T11:29:58Z

@rultor try to merge

yegor256 · 2016-01-06T11:30:03Z

@xupyprmv looks good now, thanks

rultor · 2016-01-06T11:38:38Z

@rultor try to merge

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

rultor · 2016-01-06T11:59:59Z

@rultor try to merge

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

davvd · 2016-01-07T15:17:09Z

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

davvd · 2016-01-07T15:19:53Z

@rultor deploy

rultor · 2016-01-07T15:30:01Z

@rultor deploy

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

rultor · 2016-01-07T15:41:18Z

@rultor deploy

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

elenavolokhova · 2016-01-07T15:46:13Z

@davvd Looks good!

davvd · 2016-01-07T17:07:32Z

@davvd Looks good!

@elenavolokhova thank you

davvd · 2016-01-07T17:20:31Z

@dmzaytsev just added 10 mins to @elenavolokhova (for QA), payment ID is 74003682... thanks, I just added 31 mins to your account, payment 74003698, 263 hours and 12 mins spent... review comments (c=16) added as a bonus... added +31 to your rating, now it is equal to +1501

xupyprmv added 3 commits December 26, 2015 14:20

Refactoring for `RqHref.java:83-86: RqMethod already validates Reques…

640823e

…t-Line and... yegor256#473`

annotation fix

20c6806

checkstyle modifications

f43f53b

davvd added CR @dmzaytsev 15 mins labels Dec 26, 2015

dmzaytsev reviewed Dec 28, 2015
View reviewed changes

xupyprmv added 3 commits December 28, 2015 20:27

yegor256#473 refactoring according to CR

6665b3d

smallfix

ffb721e

identation fix

dd49071

dmzaytsev reviewed Dec 28, 2015
View reviewed changes

rollback

b66a214

xupyprmv mentioned this pull request Jan 3, 2016

RqHref.java:83-86: RqMethod already validates Request-Line and... #473

Closed

refactoring for yegor256#473 according to @yegor256 CR

a9d7c99

yegor256 reviewed Jan 4, 2016
View reviewed changes

xupyprmv added 2 commits January 4, 2016 19:44

yegor256#473 refacroting according to @yegor256 CR

a28422d

check style modifications

a961981

yegor256 reviewed Jan 4, 2016
View reviewed changes

yegor256#473 more FP

c343a53

rultor merged commit c343a53 into yegor256:master Jan 6, 2016

#473 Refactoring Request-Line validation #484

#473 Refactoring Request-Line validation #484

Conversation

xupyprmv commented Dec 26, 2015

xupyprmv commented Dec 26, 2015

davvd commented Dec 26, 2015

davvd commented Dec 26, 2015

dmzaytsev commented Dec 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmzaytsev commented Dec 28, 2015

xupyprmv commented Dec 28, 2015

Choose a reason for hiding this comment

dmzaytsev commented Dec 28, 2015

xupyprmv commented Dec 28, 2015

dmzaytsev commented Dec 28, 2015

dmzaytsev commented Dec 28, 2015

rultor commented Dec 28, 2015

xupyprmv commented Jan 4, 2016

Choose a reason for hiding this comment

yegor256 commented Jan 4, 2016

xupyprmv commented Jan 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yegor256 commented Jan 4, 2016

xupyprmv commented Jan 4, 2016

yegor256 commented Jan 6, 2016

yegor256 commented Jan 6, 2016

rultor commented Jan 6, 2016

rultor commented Jan 6, 2016

davvd commented Jan 7, 2016

davvd commented Jan 7, 2016

rultor commented Jan 7, 2016

rultor commented Jan 7, 2016

elenavolokhova commented Jan 7, 2016

davvd commented Jan 7, 2016

davvd commented Jan 7, 2016