[DEV] Implementation of Hash Table Data Structure #1627

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

Jump to bottom

Open

miladsade96 wants to merge 5 commits into TheAlgorithms:master

from miladsade96:devHashTable

Open

[DEV] Implementation of Hash Table Data Structure #1627

miladsade96 wants to merge 5 commits into TheAlgorithms:master from miladsade96:devHashTable

Conversation

miladsade96

Copy link

@miladsade96 miladsade96 commented Mar 1, 2024

Open in Gitpod know more

Describe your change:

Adding a data structure called hash table

miladsade96 added 4 commits

March 1, 2024 12:19

@miladsade96


 Creating HashTable.js and HashTable.test.js files

1fdf350

@miladsade96


 Installing jest

@miladsade96


 Implementing HashTable class

0089b70

@miladsade96


 Testing HashTable class

40dbe92

@miladsade96 miladsade96 requested review from raklaptudirm and appgurueu as code owners

March 1, 2024 13:05

raklaptudirm

raklaptudirm reviewed

Mar 2, 2024

View reviewed changes

Copy link

Member

@raklaptudirm raklaptudirm left a comment

Choose a reason for hiding this comment

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

Needs way more comments to properly explain a concept like hash tables.

@miladsade96

Copy link

Author

miladsade96 commented Mar 2, 2024

@raklaptudirm OK

@miladsade96


 Improving the documentation - How hash table works

d05deeb

@codecov-commenter

Copy link

codecov-commenter commented Mar 2, 2024

Codecov Report

Attention: Patch coverage is 80.95238% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 83.88%. Comparing base (8734dfc) to head (d05deeb).
Report is 2 commits behind head on master.

Files	Patch %	Lines
Data-Structures/HashTable/HashTable.js	80.95%	20 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

appgurueu

appgurueu reviewed

Mar 2, 2024

View reviewed changes

Copy link

Collaborator

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

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.

Data-Structures/HashTable/HashTable.js

}

// Hash function:

_hash(key, max) {

Copy link

Collaborator

@appgurueu appgurueu Mar 2, 2024

Choose a reason for hiding this comment

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

Use the # syntax for private properties. The max parameter is also unnecessary; you can just get it as this.storage.length instead.

Data-Structures/HashTable/HashTable.js

if (this.storage[index][i][0] === key) {

// If the key exists, update the value

this.storage[index][i][1] = value

inserted = true

Copy link

Collaborator

@appgurueu appgurueu Mar 2, 2024

Choose a reason for hiding this comment

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

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).

Data-Structures/HashTable/HashTable.js

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++) {

Copy link

Collaborator

@appgurueu appgurueu Mar 2, 2024

Choose a reason for hiding this comment

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

Use a for-of loop here: for (const entry of this.storage[index]).

Data-Structures/HashTable/HashTable.js

export default class HashTable {

constructor(limit = 100) {

// Initialize the storage and limit variables

this.storage = new Array(limit)

Copy link

Collaborator

@appgurueu appgurueu Mar 2, 2024

Choose a reason for hiding this comment

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

It would be helpful to call this property this.#buckets (it should also be private).

Data-Structures/HashTable/HashTable.js

constructor(limit = 100) {

// Initialize the storage and limit variables

this.storage = new Array(limit)

this.limit = limit

Copy link

Collaborator

@appgurueu appgurueu Mar 2, 2024

Choose a reason for hiding this comment

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

This field is redundant with this.buckets.length.

Data-Structures/HashTable/HashTable.js

@@ -0,0 +1,105 @@

export default class HashTable {

constructor(limit = 100) {

Copy link

Collaborator

@appgurueu appgurueu Mar 2, 2024

Choose a reason for hiding this comment

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

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.

Data-Structures/HashTable/HashTable.js

// Hash the key

const index = this._hash(key, this.limit)

// Check if the bucket exists

if (this.storage[index]) {

Copy link

Collaborator

@appgurueu appgurueu Mar 2, 2024

Choose a reason for hiding this comment

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

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.

Data-Structures/HashTable/HashTable.js

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

Copy link

Collaborator

@appgurueu appgurueu Mar 2, 2024

Choose a reason for hiding this comment

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

(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)

Data-Structures/HashTable/HashTable.js

// 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]

Copy link

Collaborator

@appgurueu appgurueu Mar 2, 2024

Choose a reason for hiding this comment

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

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.

Data-Structures/HashTable/HashTable.js

}

// Print all keys/values in the table

printTable() {

Copy link

Collaborator

@appgurueu appgurueu Mar 2, 2024

Choose a reason for hiding this comment

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

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.

Labels

None yet

4 participants

@miladsade96 @codecov-commenter @appgurueu @raklaptudirm

Uh oh!

[DEV] Implementation of Hash Table Data Structure #1627

Are you sure you want to change the base?

[DEV] Implementation of Hash Table Data Structure #1627

Uh oh!

Conversation

@miladsade96 miladsade96 commented Mar 1, 2024

Describe your change:

Uh oh!

@raklaptudirm raklaptudirm left a comment

Choose a reason for hiding this comment

Uh oh!

miladsade96 commented Mar 2, 2024

Uh oh!

codecov-commenter commented Mar 2, 2024

Codecov Report

Uh oh!

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Mar 2, 2024

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Mar 2, 2024

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Mar 2, 2024

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Mar 2, 2024

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Mar 2, 2024

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Mar 2, 2024

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Mar 2, 2024

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Mar 2, 2024

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Mar 2, 2024

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Mar 2, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!