erikras / lru-memoize

A utility to provide LRU memoization for any js function
MIT License
316 stars 20 forks source link

slice → splice while moving entry to the top #88

Closed heydiplo closed 5 years ago

heydiplo commented 6 years ago

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 commented 6 years ago

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%> (ø) :arrow_up:

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 commented 6 years ago

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

heydiplo commented 6 years ago

@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 commented 6 years ago

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

polarathene commented 5 years ago

Why hasn't this been merged? @erikras

erikras commented 5 years ago

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