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.

Sign up for GitHub

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

Jump to bottom

change from slice to map #70

Merged
merged 1 commit into from
Nov 20, 2019
Merged

change from slice to map #70

merged 1 commit into from
Nov 20, 2019

Conversation

pikomonde
Copy link
Contributor

Fixes #67

Changes:

  • change some of previous declaration from slice of AppConfig into hashmap.

How to test this PR:

  • haven't run the unit test
  • should we do benchmarking to know that is it really faster using these map?

nkryuchkov
nkryuchkov reviewed Nov 14, 2019
View reviewed changes
pkg/visor/visor.go Outdated Show resolved Hide resolved
nkryuchkov
nkryuchkov reviewed Nov 15, 2019
View reviewed changes
Copy link
Contributor

@nkryuchkov nkryuchkov left a comment

Choose a reason for hiding this comment

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

Looks good! We can also do a lookup for an app by name instead of iterating all apps in (*Node).StartApp

nkryuchkov
nkryuchkov reviewed Nov 18, 2019
View reviewed changes
Copy link
Contributor

@nkryuchkov nkryuchkov left a comment

Choose a reason for hiding this comment

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

Good job! Just a small request :)

pkg/visor/visor.go Outdated Show resolved Hide resolved
@pikomonde pikomonde force-pushed the changeFromSliceToMap branch from 235e2f4 to 64d4f57 Compare November 18, 2019 18:52
@pikomonde pikomonde changed the base branch from milestone2 to master November 18, 2019 18:53
@pikomonde pikomonde marked this pull request as ready for review November 19, 2019 07:29
@pikomonde pikomonde force-pushed the changeFromSliceToMap branch 2 times, most recently from c9ba902 to 64d4f57 Compare November 19, 2019 15:26
@pikomonde pikomonde closed this Nov 19, 2019
@pikomonde pikomonde force-pushed the changeFromSliceToMap branch from 64d4f57 to 8a66b88 Compare November 19, 2019 15:28
@pikomonde pikomonde reopened this Nov 19, 2019
nkryuchkov
nkryuchkov approved these changes Nov 20, 2019
View reviewed changes
Copy link
Contributor

@nkryuchkov nkryuchkov left a comment

Choose a reason for hiding this comment

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

Looks good to me, well done!

@jdknives jdknives merged commit ed12183 into skycoin:master Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkryuchkov nkryuchkov nkryuchkov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use maps instead of slices where lookups often happen
3 participants
@pikomonde @nkryuchkov @jdknives