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

[super132]

For task #160 , added Concat class and another utility class IterableTransform for header concatenation.

[super132]

@davvd , please review the pull request.

[davvd]

@super132 Let me find a reviewer for this pull request, thanks for submitting it

[davvd]

@krzyk review this one,please

[krzyk]

@super132 please merge changes from master to this PR

[krzyk]

@super132 why create a storage if we can reuse the original iterables for this? This will waste more memory.

[super132]

@krzyk , may I know if you mean to initialise it lazily or this storage is not even needed? If it is the later case then how could we reuse storage as this class should not holds any reference to the input iterables.

[krzyk]

@super132 why shouldn't this class hold references to the input iterables?

[super132]

@krzyk, I have questions regarding to holding references as I believe the iterables could be modified outside using remove() method and the ultimate memory usage would not be higher:

1. How are we implement iterator() method? Do we need to write a brand new iterator which calls the iterators of the two iterables?
2. How are the use cases of Concat? If there are more use cases that the input iterable will eventually be garbage collected then I think creating one new object would save more memory. I think when we use Concat, we would be more interested in the end result rather than the input and the input would then be unreferenced.
3. How would we handle remove() method that could be implemented by iterables? If the input iterable is modified by this method then it would affect the concatenated result if we rely on the reference to produce the output.

Thanks.

[krzyk]

@super132
For now let's leave it as it is and will see what @yegor256 thinks about current design.

1. You would create a new iterator that would iterate over the first iterable and after it finishes it would go to the other one
2. I don't know that.
3. Remove would just throw UnsupportedOperationException as it does in some iterators in JDK.

[super132]

@krzyk, no problem. Thanks.

[yegor256]

@super132 well, you're not solving the key problem with this implementation. At the moment Concat class is fetching items from all iterables during constructing. What if they have 1bln item each and the user will do: new Concat(bln, bln).iterator().next(); You will fetch 2 bln items, while the user needs just one.

[super132]

@yegor256, understood. I would change the implementation for it.

[krzyk]

@super132 please see my comments above

[super132]

@krzyk, thanks for your comments. I have updated some in the commit. I have some questions regarding to your comments and would you please help take a look of it? Thanks.

[super132]

@yegor256, instead of using Transform, I modifed LowerCase to be a decorative Condition which apply the supplied Condition by changing the element into lower case. StartsWith condition is created for checking prefix. I think in that way, we can have every Condition does one single action.
I have also refactored SelectIterator to extends Guava AbstractIterator to have more clean implementation. Would you please review? Thanks.

[yegor256]

we can't use any dependencies in this project

[super132]

No problem. I can revert back to previous version.

[yegor256]

@super132 again all wrong :) I would suggest you to first draw the design in text and suggest it. we'll discuss. then you will create Java code. looks like you don't understand the idea of functional programming and its application in OOP. I asked you whether you used Guava just to check whether you have any experience in using their "functional" concepts. besides that, your branch is very big in size, I would recommend to make it shorter and use PDD: http://www.yegor256.com/2009/03/04/pdd.html

[super132]

@yegor256, I thought it would be ok this time :P I still do not think that the lower case here is a transformation. Let's take a look of the original codes in RqWithoutHeader:

for (final String header : req.head()) {
    if (!header.toLowerCase(Locale.ENGLISH)
        .startsWith(prefix)) {
        head.add(header);
    }
}

The logic here is to compare if header does not contain a prefix in a case insensitive manner and output it. And once it passes the condition, the original header, not the one transformed into lower case, is added to the result. Same logic here in the current implementation that I use a Condition to do this comparison by translating the header to lower case for case insensitive comparison. I do not see why do you think it is a transformation. Would you please point me to that? Would it be better to rename it to StartsWithIgnoreCase instead of LowerCase ?

If we had to implement it using transformation, then we have to do something like this:

new Transform<String, String>(
    new Select<String>(
        new Transform<String, String>(
            req.head(),
            new TransformAction.LowerCase()
        ),
        new Condition.Not<String>(
            new Condition.StartsWith(prefix)
        )
    ),
    new TransformAction.NormalCase()
)

The problems in the above implementation are:

1. The implementation of NormalCase. How do we implement this transformation? LowerCase transformation is destructive and cannot be reverted unless we save a copy first and compare the result transformed. But it would then defeat the purpose of this design being very light-weight and do not materialize iterable except the original input.
2. The above transformation would be slower than doing the case insensitive comparison in one Condition as it involves 2 transformations and one select. In my current implementation it involves one select only.

As per your comment preivously, this case specific condition should be inline. I generally agree on this approach but what I need to do is to make an inner class in RqWithoutHeader and RsWithoutHeader to implement the inline condition as qulice complains about the number of lines in the anonymous inner classes that these two classes are using now.

Based on the reasons above, I cannot see this comparison is a transformation, but a case insensitive comparison. If we really needed to implement it in transformation, would you please advise on how to implement transformation that can revert an iterable in lower case to its original case? Should we use inline approach instead of making them general conditions here? Thanks.

[super132]

@yegor256, I am happy to create a puzzle for the case insensitive transformation implementaiton. Should I modify my codes based on my comments above (grouping the use of toLowerCase() and startsWith() in one condition) and make it a puzzle instead? Thanks.

[yegor256]

@super132 don't worry about qulice complaining about too long anonymous class. You can just suppress this warning, as it's done, for example in TkFallback, like 108.

I would do it like this:

new Select<String>(
  req.head(),
  new Condition<String>() {
    @Override
    public boolean fits(String header) {
      return header.toLowerCase().startsWith(prefix);
    }
  }
)

[super132]

@yegor256 Thanks for your advice. I have moved the condition to be inline. Please have a check.

[super132]

@yegor256, may I know if the latest change is fine for you? Thanks.

[super132]

@yegor256 , may I have your opinion on it? It has been pending for 4 days. Thanks.

[yegor256]

@super132 I'll update you tomorrow, sorry for the delay

[yegor256]

@super132 take a look: http://www.yegor256.com/2015/04/30/iterating-adapter.html

[super132]

@yegor256 , do you mean that I need to implement the Iterator to take in the data source directly instead of the iterator and copy the whole element into a buffer instead? So the ConcatIterator for example will become something like below?

class ConcatIterator<E> implements Iterator<E> {
    private final Iterable<E> aitr;
    private final Iterable<E> bitr;
    private final Queue<E> buffer = new LinkedList<E>();
    public ConcatIterator(final Iterable<E> alist, final Iterable<E> blist) {
        this.aitr = alist;
        this.bitr = blist;
    }
    public boolean hasNext() {
        if (this.buffer.isEmpty()) {
            //iterate over aitr and bitr and save the item into queue
        }
        return !this.buffer.isEmpty();
    }
    public E next() {
         if (!this.hasNext()) {
             throw new NoSuchElementException("No element");
         }
         return this.buffer.poll();
    }
}

But then will it violate the rule you told me before that the iterable and the iterator should be as lazy as possible? The implementation now will do the actual condition check and transformation at the very last moment and using the implementation above, it would not be this lazy and it will scan through all elements even though the caller gets the first element in the end. Or do you want to change the implementation to take in the Iterable instead instead of the Iterator directly?

Another question I have about the implementation of hasNext(). If we use the approach above, should I get a new iterator in hasNext() method for the Iterable we saved? But then it would never stop as Iterable.iterator() method always returns an Iterator with index set at zero, which will then loop through the whole list again. If we are going to save the Iterator on the instantiation of this object then it would be no different from the current implementation except taking the Iterables as the constructor input and copy all the elements over from the Iterables in hasNext().

With the questions above, may I confirm with you that would you want to have the implementation like the code block above? Thanks.

[yegor256]

@super132 we're making this pull request too long. You're asking too many questions and I have to spend too much time to educate you. I would suggest to split this pull request into smaller parts. This one is too big and has too many issues.

[super132]

@yegor256 , no problem. I will create puzzle on the Iterator design in all 3 iterator that I made and close this one. Would it be better to handle the issues like this? I'm going to do add the puzzle comments if you have no objection. Thanks.

[yegor256]

@super132 in general, we prefer short branches

[super132]

@yegor256 , let me split this PR into 3 smaller PRs, which will contain Concat, Select and Transform respectively and their affected classes.

[super132]

@yegor256 , I have split this PR into 3 smaller PRs (#232, #233 and #234). Shall we close this PR and discuss in the child PRs? Thanks.

[yegor256]

@super132 yes, I think this one should be closed

[davvd]

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

[elenavolokhova]

@davvd This request wasn't merged but performer should be rewarded for review. Quality is acceptable.

[davvd]

@davvd This request wasn't merged but performer should be rewarded for review. Quality is acceptable.

@elenavolokhova thanks a lot, next time everybody should try to make it better

[davvd]

@davvd This request wasn't merged but performer should be rewarded for review. Quality is acceptable.

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

[davvd]

@krzyk just added 10 mins to @elenavolokhova (for QA), payment ID is 56851691... Much appreciated! 21 mins was added to your account, payment ID 56851703, time spent is 334 hours and 58 mins... you're getting extra minutes here (c=6)... +21 added to your rating, current score is: +4914