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

slice → splice while moving entry to the top #88

Merged
merged 1 commit into from
Feb 12, 2019

Conversation

heydiplo
Copy link
Contributor

@heydiplo heydiplo commented Apr 2, 2018

I guess it's just a typo. We want an item to be removed at the old index and inserted at 0 on the next line. splice does that, slice does nothing.

@codecov-io
Copy link

codecov-io commented Apr 2, 2018

Codecov Report

Merging #88 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master     #88   +/-   ##
======================================
  Coverage    96.2%   96.2%           
======================================
  Files           4       4           
  Lines          79      79           
======================================
  Hits           76      76           
  Misses          3       3
Impacted Files Coverage Δ
src/lruCache.js 92.85% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9b4b3d...f06fcf0. Read the comment docs.

@jimmytuc
Copy link

I'm sorry just a wrong click to approve, pls dismiss it. Thanks

@heydiplo
Copy link
Contributor Author

@jimmytuc can you please explain why you want to dismiss? I can see the bug still present in a master branch.

This is current version:

var entries = [{key: 1, value: 1}, {key: 2, value: 2}, {key: 3, value: 3}]
var key = 2
var cacheIndex = 1
var entry = entries[cacheIndex]

entries.slice(cacheIndex, 1)
entries.unshift(entry)
console.log(entries)
// [{key: 2, value: 2}, {key: 1, value: 1}, {key: 2, value: 2}, {key: 3, value: 3}]

This is a fixed one:

var entries = [{key: 1, value: 1}, {key: 2, value: 2}, {key: 3, value: 3}]
var key = 2
var cacheIndex = 1
var entry = entries[cacheIndex]

entries.splice(cacheIndex, 1)
entries.unshift(entry)
console.log(entries)
// [{key: 2, value: 2}, {key: 1, value: 1}, {key: 3, value: 3}]

@jimmytuc
Copy link

hi @heydiplo, you're right after I checked on this. Thanks

@polarathene
Copy link

polarathene commented Jan 18, 2019

Why hasn't this been merged? @erikras

@erikras
Copy link
Owner

erikras commented Feb 12, 2019

Wow. That's the worst kind of javascript typo. i.e. good interview question. 🤦‍♂️

@erikras erikras merged commit bb42316 into erikras:master Feb 12, 2019
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.

5 participants