Reimplement the 'SequenceEqual' operator

[zsxwing]

Hi, this PR reimplemented the SequenceEqual operator #76 and should fix the issue #564.

The wiki page https://github.com/Netflix/RxJava/wiki/Observable-Utility-Operators#sequenceequal still needs to be updated. @DavidMGross could you help update the marble diagram of SequenceEqual? Thanks!

[cloudbees-pull-request-builder]

RxJava-pull-requests #512 SUCCESS
This pull request looks good

[DavidMGross]

Okay; I think I've got it corrected. It only covers one condition, so might
benefit from having some more examples, but it'll do for now.

On Fri, Dec 6, 2013 at 7:13 AM, CloudBees pull request builder plugin <
[email protected]> wrote:

RxJava-pull-requests #512https://netflixoss.ci.cloudbees.com/job/RxJava-pull-requests/512/SUCCESS
This pull request looks good

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/575#issuecomment-30001152
.

David M. Gross
PLP Consulting

[zsxwing]

@DavidMGross Looks cool. Thanks.

[cloudbees-pull-request-builder]

RxJava-pull-requests #514 SUCCESS
This pull request looks good

[akarnokd]

Good job, just a few things.

• You are using gate to ensure concurrency, and you access the firstDone and secondDone while synchronizing over gate. I believe these two can be simple boolean fields instead of AtomicBooleans.
• You are setting the firstValues and secondValues in the init method into a volatile fields. I believe these can be moved into the constructor and set to a regular private final List type and accessor.
• [Optional] you might want to call clear on both firstValues and secondValues if there was a termination condition so they don't hold onto queued values until the client observer disappears.

[samuelgruetter]

I really think that sequenceEqual should be implemented in terms of other operators. Don't reinvent the wheel with every operator you add ;-)
In Scala, I'd do it like this:

def sequenceEqual[T](o1: Observable[T], o2: Observable[T]): Observable[Boolean] = {
  val obs1: Observable[Option[T]] = o1.map(Some(_)) ++ Observable(None) // ++ is concat
  val obs2: Observable[Option[T]] = o2.map(Some(_)) ++ Observable(None)
  // If different length, comparing the `None` of the shorter with `Some(element)` 
  // of the longer will return false.
  (obs1 zip obs2).forall(pair => pair._1 == pair._2)
}

[zsxwing]

@samuelgruetter , great idea. Really thanks. I overrode the previous commit. As their is no Option in Java, I use Notification instead.

[cloudbees-pull-request-builder]

RxJava-pull-requests #515 SUCCESS
This pull request looks good

[zsxwing]

I also have a question about Notification.hasValue.

    /**
     * Retrieves a value indicating whether this notification has a value.
     * 
     * @return a value indicating whether this notification has a value.
     */
    public boolean hasValue() {
        return isOnNext() && value != null;
    }

Here it checks if value is not null. But I think value can be null.

I also checked the Rx.Net codes here: http://rx.codeplex.com/SourceControl/latest#Rx.NET/Source/System.Reactive.Core/Reactive/Notification.cs

            /// <summary>
            /// Returns true.
            /// </summary>
            public override bool HasValue { get { return true; } }

It returns true directly in OnNextNotification.

If Notification.hasValue has the same meaning in RxJava and Rx.Net. I think && value != null needs to be removed.

[cloudbees-pull-request-builder]

RxJava-pull-requests #516 SUCCESS
This pull request looks good

[samuelgruetter]

Much nicer now ;-)

Regarding hasValue: I don't know. What is it good for if it's the same as isOnNext? And why would one need the current version? Imho it could just be removed.

Something else regarding null values and sequenceEqual with default equality: I think you shouldn't call onError(NullPointerException), but just onNext(true) or onNext(false).

[zsxwing]

@samuelgruetter Thanks for reminding me the null issue.

[cloudbees-pull-request-builder]

RxJava-pull-requests #517 SUCCESS
This pull request looks good