Skip to content
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

perf: sortBy / orderBy #878

Merged
merged 3 commits into from Dec 30, 2018

Conversation

@dated
Copy link
Contributor

commented Dec 28, 2018

Proposed changes

  • replace sortBy with orderBy in findLatestTransaction:
    replacement for #832, uses orderBy so complexity should stay at O(logn) instead

  • replace sortBy with orderBy in merge-table-transactions.js:
    uses orderBy instead of sortBy so reversing the list is unnecessary

  • replace orderBy with sortBy ContactAll.vue:
    since contacts with the same name are not allowed, ordering by address as well is unnecessary

Types of changes

  • Refactoring (improve a current implementation without adding a new feature or fixing a bug)

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes

dated added some commits Dec 28, 2018

@faustbrian faustbrian merged commit 9a7c52d into ArkEcosystem:develop Dec 30, 2018

1 check passed

ci/circleci: test-node-9 Your tests passed on CircleCI!
Details
@faustbrian faustbrian referenced this pull request Dec 30, 2018
3 of 3 tasks complete

@dated dated deleted the dated:sorting branch Dec 30, 2018

@kalgoop

This comment has been minimized.

Copy link
Contributor

commented Dec 30, 2018

@dated how using order by keeps O(logn). I think it will take same O(nlogn) as earlier implementation. Sorting/ordering takes O(nlogn) and picking first element takes O(1). So overall it is O(nlogn)

Instead if you traverse all the elements of transactions to find max timestamp it takes O(n) only. (As in #832 )

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Dec 30, 2018

You are correct, that was a typo on my end.

@kalgoop

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2019

One of your commit actually reverts the #840 .

PHANTOM-DEV1 added a commit to PhantomChain/desktop-wallet that referenced this pull request Jan 9, 2019

perf: sortBy / orderBy (ArkEcosystem#878)
* perf: replace sortBy with orderBy in findLatestTransaction

* perf: replace sortBy with orderBy in merge-table-transactions.js

* perf: replace orderBy with sortBy ContactAll.vue

PHANTOM-DEV1 added a commit to PhantomChain/desktop-wallet that referenced this pull request Jan 17, 2019

perf: sortBy / orderBy (ArkEcosystem#878)
* perf: replace sortBy with orderBy in findLatestTransaction

* perf: replace sortBy with orderBy in merge-table-transactions.js

* perf: replace orderBy with sortBy ContactAll.vue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.