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

let's get rid of List.add() #160

Closed
yegor256 opened this issue Apr 8, 2015 · 23 comments
Closed

let's get rid of List.add() #160

yegor256 opened this issue Apr 8, 2015 · 23 comments

Comments

@yegor256
Copy link
Owner

yegor256 commented Apr 8, 2015

We're using add() method in a few places, from List interface, mostly for concatenation of headers. Let's get rid of that and introduce a new class org.takes.misc.Concat, which implement Iterable and encapsulate two iterables inside:

Iterable<String> list = new Concat<>(
  response.head(),
  Collections.singleton("Host: www.example.com")
);
@davvd
Copy link

davvd commented Apr 9, 2015

@yegor256 thanks, I added the "bug" tag

@davvd davvd added the bug label Apr 9, 2015
@davvd
Copy link

davvd commented Apr 9, 2015

@yegor256 thanks a lot for reporting, 15 mins added to your acc, pmt ID 000-6b1af8f3

@davvd
Copy link

davvd commented Apr 15, 2015

@super132 the task is for you now, follow these guidelines. Don't hesitate to ask any technical questions right here... The budget of this issue is 30 mins, which is exactly how much will be paid when the task is done (see this for explanation)

@super132
Copy link
Contributor

@yegor256 , may I know could you please point me an example that illustrating this bug? Do you mean to replace the List interface use in the whole code base or just replace the use of List interface specifically for concatenating headers only?

Thanks.

@super132
Copy link
Contributor

@yegor256 After digging into the codes more, my understanding is to replace the use of List as a tool to concatenate headers string by Concat class. If my understand is correct then I will start work on this.

Thanks.

@yegor256
Copy link
Owner Author

@super132 this ticket is about concatenating headers only. You're right, we should use Concat class instead of manual concatenation of headers with List.add(). For example, this code in RsWithHeaders:

        final List<String> headers = new LinkedList<String>();
        for (final String hdr : head) {
            headers.add(hdr);
        }
        headers.add(header);
        return headers;

Should be replaced with:

return new Concat(head, Collections.singleton(headers));

@super132
Copy link
Contributor

@davvd PR #183 has been created for this task. Please review.

@davvd
Copy link

davvd commented May 1, 2015

@super132 check this "no obligations principle".. This task is on your name for at least 15 days. If you can't close it within the next 48 hours I will have to assign someone else to it. This article should help if you're stuck

-30 added to your rating, at the moment it is: +8

@super132
Copy link
Contributor

super132 commented May 1, 2015

@davvd , there is no bug in my codes. I will try my best to get the code review passed in my next 48 hours.

@super132
Copy link
Contributor

super132 commented May 2, 2015

@yegor256, since I have split the solutions into 3 pull requests and #232 can address this issue. Can I create another issue for dealing the case of filtering and transformation that #233 and #234 try to solve? Thanks.

@yegor256
Copy link
Owner Author

yegor256 commented May 4, 2015

@super132 we don't do it like this. we use PDD instead: http://www.yegor256.com/2009/03/04/pdd.html You created too many pull requests for this single ticket. This is a wrong approach in general. You should try to write as little code as possible and mark everything that can be done later with @todo puzzles.

@super132
Copy link
Contributor

super132 commented May 5, 2015

@yegor256 I learned when should create puzzles from this ticket. I will do it in a much better way for future tickets. Thanks.

@davvd
Copy link

davvd commented May 9, 2015

@yegor256 there is a puzzle in this code 160-f5eebf68/#268, we'll resolve it later

@super132
Copy link
Contributor

@yegor256 , PR #232 #233 and #234 fix this issue. Please check and close this ticket. Thanks.

@yegor256
Copy link
Owner Author

@super132 thanks!

@davvd
Copy link

davvd commented May 11, 2015

@elenavolokhova please, let us know what do you think about this ticket, according to our QA rules

@elenavolokhova
Copy link

@super132
According to our quality rules:

Messages in a ticket always start with a name of a user they are addressed to.

There are some comments in this ticket with no addressee. Try to avoid such issues in future.
Please confirm that everything is clear and you have no questions regarding this approach.

@super132
Copy link
Contributor

@elenavolokhova I fixed comments without addressees. I confirm I am clear and no question on the QA rule. Thanks.

@elenavolokhova
Copy link

@super132 Thank you!

@elenavolokhova
Copy link

@davvd Quality is acceptable.

@davvd
Copy link

davvd commented May 12, 2015

@davvd Quality is acceptable.

@elenavolokhova thanks, got it. everybody, please try to make it good next time

@davvd
Copy link

davvd commented May 12, 2015

@super132 10 mins was added to the account of @elenavolokhova (for QA review), in transaction 57184341... Much obliged! I have added 1 hour to your account in payment "AP-3MG05873T77364832", 599 hours and 7 mins spent... age=2, that's why a bonus here for fast delivery... +60 added to your rating, at the moment it is: +83

@davvd
Copy link

davvd commented Jun 14, 2015

@yegor256 the last puzzle 160-fc7f658c/#276 solved

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

No branches or pull requests

4 participants