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

feat : added tries implementation using typescript #150

Merged
merged 7 commits into from
Sep 29, 2023

Conversation

Suryac72
Copy link
Contributor

No description provided.

Copy link
Contributor

@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 also get rid of your changes to package-lock.json.

data_structures/tries/test/tries.test.ts Outdated Show resolved Hide resolved
data_structures/tries/tries.ts Outdated Show resolved Hide resolved
data_structures/tries/tries.ts Outdated Show resolved Hide resolved
data_structures/tries/tries.ts Outdated Show resolved Hide resolved
data_structures/tries/tries.ts Outdated Show resolved Hide resolved
data_structures/tries/tries.ts Outdated Show resolved Hide resolved
data_structures/tries/tries.ts Outdated Show resolved Hide resolved
data_structures/tries/tries.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@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 get rid of redundant comments (practically all of your comments currently). A comment is redundant when all it says is already found in the code. For example:

  /**
   * 
   *
   * @param word 
   * @param word
   * @param position 
   * @param position
   * @param letterMap 
   * @param letterMap
   * @returns 
   * @returns
   * 
   * Private method implementation to search word in letterMap of type (Trie)
   */

This is redundant with the signature (and outdated even).

I'm fine with omitting comments if a method is self-explanatory (add and find don't need comments; I'd rename find to has however, and isPrefixMatch should be documented).

@Suryac72
Copy link
Contributor Author

hi, @appgurueu , I removed redundant comments and package-lock.json file from my PR, please have a look. also, I document isPrefixMatch flag variable in the form of a comment, please have a look. If further modification needs to be done, let me know.

thanks for figuring out my mistakes and reviewing my PR

appgurueu
appgurueu previously approved these changes Sep 25, 2023
@Suryac72
Copy link
Contributor Author

Hi @raklaptudirm, please review my PR.

@raklaptudirm
Copy link
Member

Your tests are failing. Also undo the deletion of the package-lock file.

@Suryac72
Copy link
Contributor Author

hi @raklaptudirm, I updated my PR

Copy link
Contributor

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

Undo the changes, not delete the file.

@Suryac72
Copy link
Contributor Author

Hi @appgurueu which changes i need to undo?

@Suryac72
Copy link
Contributor Author

Hi @raklaptudirm, I can see all checks have passed successfully

@raklaptudirm
Copy link
Member

You need to undo the deletion of the package-lock.json file.

@Suryac72
Copy link
Contributor Author

Hi @raklaptudirm, I added again package-lock.json file

@raklaptudirm
Copy link
Member

Add the file without changes, please.

@Suryac72
Copy link
Contributor Author

hi @raklaptudirm I added package-lock.json without any changes. just like present in master branch

@Suryac72
Copy link
Contributor Author

Hi @raklaptudirm , @appgurueu approved my PR. Please approve my PR from your end

@Suryac72
Copy link
Contributor Author

hi @raklaptudirm, my PR was approved by @appgurueu. please review my PR so that I can able to merge my PR

@appgurueu
Copy link
Contributor

Please be patient. Rak will get to review your PR when he has time.

@raklaptudirm raklaptudirm merged commit 24b02c1 into TheAlgorithms:master Sep 29, 2023
2 checks passed
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.

3 participants