collectable icon indicating copy to clipboard operation
collectable copied to clipboard

Question: is the built-in hash function sufficient?

Open njlr opened this issue 7 years ago • 1 comments

I was looking through the source and found that the hash function is quite simple:

import { stringHash } from '../common';

export function findHash (key: any): number {
  const hash: number = typeof key === 'number'
    ? key
    : typeof key === 'string'
      ? stringHash(key)
      : Math.abs(stringHash(JSON.stringify(key)));

  return hash;
}

I can see two problems that might occur with this.

First, JSON.stringify can be sensitive to property order, which might lead to unexpected behaviour:

> JSON.stringify({ x: 1, y: 2 })
'{"x":1,"y":2}'
> JSON.stringify({ y: 2, x: 1 })
'{"y":2,"x":1}'

Second, (and please correct me if I missed something!) it does not leave a possibility for customization. I think the set and map modules should be parameterized on a tuple of hash and equals. A default can be provided for convenience.

njlr avatar Feb 26 '18 18:02 njlr

Hey, really sorry about the slow reply. I foolishly read the issue on my phone before going to sleep, and then forgot to follow up.

The HashMap collection was actually ported from another package (see the contributor credits), and the intent was to upgrade the hashing function there to use what's in the core library. It hasn't come up as an issue for me yet though, which is why it's remained on the backburner. I'm open to pull requests on this one. Note that what's in the core library should be ok in theory, but again if you see any issues there, I'm very much open to contributions that help me improve things. This project is very much alive, despite the apparent long periods of inactivity - I just have several projects I'm working on in tandem, and being just one person, I only have so much productive bandwidth to allocate at any given time.

axefrog avatar Mar 07 '18 22:03 axefrog