Skip to content
This repository has been archived by the owner on Feb 8, 2022. It is now read-only.

Speed up the initial fetch #58

Merged
merged 11 commits into from
Jan 12, 2016

Conversation

bronenos
Copy link
Contributor

@bronenos bronenos commented Jan 8, 2016

If we have about 10k objects, we don't have to sort these objects 10k times.
So, lets sort each section just once after all relative objects have been inserted.

return section
}

Set(sections).forEach {
Copy link
Owner

Choose a reason for hiding this comment

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

One liner .forEach { $0.sort() } please 🙏

@isaacroldan
Copy link
Contributor

The idea is interesting, but i think you are abusing map, you are creating an array of Sections the same size of the Objects array, and that doesn't make sense, forcing you to then do a set later.

My solution would be:

objects.forEach { getOrCreateSection($0).insert($0) }
sections.forEach { $0.sort() }

What do you think?

@bronenos
Copy link
Contributor Author

bronenos commented Jan 8, 2016

Oh, yes, I forgot about internal sections.
The only problem is - if we will have two sections, each with 10k objects, then in case of filling just one section, the second would be sorted too?

If not, I agree with you.

@polqf
Copy link
Owner

polqf commented Jan 8, 2016

@bronenos When a section runs out of objects, it is removed. So if a user only has 10k elements in one sections, that's the only one that is going to be sorted

@isaacroldan isaacroldan closed this Jan 8, 2016
@isaacroldan isaacroldan reopened this Jan 8, 2016
@bronenos
Copy link
Contributor Author

bronenos commented Jan 8, 2016

Okay, well, who will add the changes we discussed above?

@polqf
Copy link
Owner

polqf commented Jan 8, 2016

Whatever you prefer @bronenos , if you cannot do it, we'll do it whenever we can 👍

@bronenos
Copy link
Contributor Author

bronenos commented Jan 8, 2016

Well, ok, I will

@isaacroldan
Copy link
Contributor

Rebase with master, there are new commits there conflicting

Also, Add tests for tests for the new insert and sort functions please :)

@bronenos
Copy link
Contributor Author

bronenos commented Jan 9, 2016

Done

it("beforeAll") {
section = Section<Task>(keyPath: "keyPath", sortDescriptors: sortDescriptors)
section.insert(openTask)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

please use the beforeEach closure.
If you need to clean between tests (and you should), use the afterEach

(apply to all tests ;) )

@polqf
Copy link
Owner

polqf commented Jan 12, 2016

Hi @bronenos ,

There are cases where you are using the it closure incorrectly, they are easily fixed by using beforeEach and afterEach when necessary

polqf added a commit that referenced this pull request Jan 12, 2016
@polqf polqf merged commit 26a4b38 into polqf:master Jan 12, 2016
@polqf
Copy link
Owner

polqf commented Jan 12, 2016

Merging it, will do a PR with the fixes commented above

Thanks @bronenos

@polqf polqf mentioned this pull request Jan 12, 2016
@bronenos
Copy link
Contributor Author

Ok, thank you.
I'm not familiar with this library, did not use it before.

@polqf polqf modified the milestone: 0.4.0 Jan 13, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants