Skip to content

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.

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