Skip to content

Added old value to .RemoveElement operation #22

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Added old value to .RemoveElement operation #22

wants to merge 3 commits into from

Conversation

olejnjak
Copy link
Contributor

Hi,

as discussed in #21, I'm creating PR now.

I've updated some more stuff, like Operation.RemoveElement now returns a value and is mappable so I've updated tests as well. Please revise them as it's not my cup of tea.

@@ -53,8 +53,8 @@ public enum Operation<T>: CustomDebugStringConvertible {
return value
case .Update(let value, _):
return value
default:
return Optional.None
case .RemoveElement(_, let oldValue):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value property no longer needs to be Optional

@guidomb
Copy link

guidomb commented Jan 27, 2016

Apart from the small comments I made there is something that bothers me. The thing is that in case you create a derived reactive array from another reactive array using the mirror method; every time an element is removed from the original array we are mapping the old value again in the other array when actually the mapped value has already been saved in the mirror's internal array.

This shouldn't be a problem if your map function is a pure function (which it should) but if for some reason your mapping operation does a heavy computation we are wasting resources.

I propose we refactor the mirror method as follows

public func mirror<U>(transformer: T -> U) -> ReactiveArray<U> {
    let mirrorProducer = producer.map { operation in 
        // This avoids having to recompute the mapped value
        // for a remove operation which is not needed because
        // we are already storing the mapped values in the
        // mirror's internal array
        if case .RemoveElement(let index, _) == operation {
            return .RemoveElement(index, _elements[index])
        } else {
            return operation.map(transformer) 
        }
    }
    return ReactiveArray<U>(producer: mirrorProducer)
}

@olejnjak
Copy link
Contributor Author

Okay, will fix and push those changes.

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

Successfully merging this pull request may close these issues.

2 participants