[DEV] Implementation of Hash Table Data Structure #1627
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1627 +/- ##
==========================================
+ Coverage 83.66% 83.88% +0.21%
==========================================
Files 377 378 +1
Lines 19733 19841 +108
Branches 2907 2933 +26
==========================================
+ Hits 16509 16643 +134
+ Misses 3224 3198 -26 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Please remove unnecessary comments (prime example: // Initialize the hash variable to 0 is plain redundant with the code).
The tests are very barebones. Ideally you should roughly try to reach decent coverage. I'd like to see some tests for collisions, for example.
A simple way to reach decent coverage would be to execute more or less random operations, comparing the result against JS's Map or a JS object.
| } | ||
|
|
||
| // Hash function: | ||
| _hash(key, max) { |
There was a problem hiding this comment.
Use the # syntax for private properties. The max parameter is also unnecessary; you can just get it as this.storage.length instead.
| if (this.storage[index][i][0] === key) { | ||
| // If the key exists, update the value | ||
| this.storage[index][i][1] = value | ||
| inserted = true |
There was a problem hiding this comment.
Just early return here. Then you don't need the inserted variable, and you will be more efficient (by not doing any further loop iterations).
| else { | ||
| // If the index is not empty, iterate through the bucket (collision handling) | ||
| let inserted = false | ||
| for (let i = 0; i < this.storage[index].length; i++) { |
There was a problem hiding this comment.
Use a for-of loop here: for (const entry of this.storage[index]).
| export default class HashTable { | ||
| constructor(limit = 100) { | ||
| // Initialize the storage and limit variables | ||
| this.storage = new Array(limit) |
There was a problem hiding this comment.
It would be helpful to call this property this.#buckets (it should also be private).
| constructor(limit = 100) { | ||
| // Initialize the storage and limit variables | ||
| this.storage = new Array(limit) | ||
| this.limit = limit |
There was a problem hiding this comment.
This field is redundant with this.buckets.length.
| @@ -0,0 +1,105 @@ | |||
| export default class HashTable { | |||
| constructor(limit = 100) { | |||
There was a problem hiding this comment.
100 seems very arbitrary. I'd force the user to choose for a fixed-size hash table. (I'd also make it clear that this is a fixed-size hash table (with dynamically grown buckets) as opposed to a dynamic hash table. In particular, a fixed-size hash table can't guarantee expected O(1) operations.)
limit is also a slightly misleading name: This is not a hard limit, just a hash table "size". If you insert more elements, you will just get worse performance as your buckets fill up.
| // Hash the key | ||
| const index = this._hash(key, this.limit) | ||
| // Check if the bucket exists | ||
| if (this.storage[index]) { |
There was a problem hiding this comment.
I'd use an early return here: const bucket = this.storage[index]; if (!bucket) return (empty / undefined bucket). Then you don't need to indent the rest of the function.
| const index = this._hash(key, this.limit) | ||
| // Check if the bucket exists | ||
| if (this.storage[index]) { | ||
| // If the key matches the key at the index and there is only one item in the bucket, delete the bucket |
There was a problem hiding this comment.
(leaving an empty bucket behind wouldn't be an issue either; in fact i think the implementation may become slightly cleaner if you initialize all buckets to be empty, at a slight upfront performance cost at initialization)
| // If the index is not empty, iterate through the bucket | ||
| for (let i = 0; i < this.storage[index].length; i++) { | ||
| // If the key exists, delete the key-value pair | ||
| if (this.storage[index][i][0] === key) delete this.storage[index][i] |
There was a problem hiding this comment.
You're creating an array with holes here. I think this is rather dirty; it results in suboptimal time complexities (deleted items negatively affecting the time it takes to search a bucket).
It would be cleaner to just take an entry from the end, swap the entry to be deleted with that, then pop the entry from the end.
| } | ||
|
|
||
| // Print all keys/values in the table | ||
| printTable() { |
There was a problem hiding this comment.
I don't think "print" helpers are good design. A debugger, or even just JSON.stringify, work just as well for inspecting values.
A better abstraction would be an "entries"-like method to iterate over all the key-value-pairs.
Describe your change: