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

Added Trie Logic #86

Merged
merged 1 commit into from
Oct 3, 2020
Merged

Added Trie Logic #86

merged 1 commit into from
Oct 3, 2020

Conversation

GajeshS
Copy link
Contributor

@GajeshS GajeshS commented Oct 26, 2019

#67 Added logic for Trie for use cases such as storing username/password.

def __init__(self):
self.start = trienode('')
def insert(self,id,data): # returns true on successful insertion (data == password)
temp = self.start
Copy link

Choose a reason for hiding this comment

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

Please use an appropriate variable name for temp
Refer to - https://www.quora.com/Why-is-using-a-variable-name-temp-considered-bad-practice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the variable name.

Copy link

Choose a reason for hiding this comment

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

The variable name is still temp. Please change it to something meaningful @GajeshS

class trie: # use case: username/password
def __init__(self):
self.start = trienode('')
def insert(self,id,data): # returns true on successful insertion (data == password)
Copy link

Choose a reason for hiding this comment

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

Please use key instead of data as a variable name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the variable name

return True
def findNode(self,id): # returns false if node doesn't exist and true, node if it does
temp = self.start
for x in id:
Copy link

@0001vrn 0001vrn Oct 28, 2019

Choose a reason for hiding this comment

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

Please use appropriate variable name for variable x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

t.insert('test2',"dummy2")
print(t.findNode('test')) # (false,None)
a,b = t.findNode('test1')
print(a,b) # (true,memref)
Copy link

Choose a reason for hiding this comment

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

Instead of printing a memref use an appropriate message.
Refer to - https://www.geeksforgeeks.org/trie-insert-and-search/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link

@0001vrn 0001vrn left a comment

Choose a reason for hiding this comment

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

Minor comments. Going forward, kindly request you to use meaningful and appropriate variable names.

Also, to understand the significance behind meaningful names
Please read chapter 2 of the book - Clean Code
Refer to https://github.com/SaikrishnaReddy1919/MyBooks/blob/master/%5BPROGRAMMING%5D%5BClean%20Code%20by%20Robert%20C%20Martin%5D.pdf

@0001vrn
Copy link

0001vrn commented Nov 24, 2019

@GajeshS
Would you be able to incorporate the code review comments?

@GajeshS
Copy link
Contributor Author

GajeshS commented Dec 4, 2019

Hey,
Sorry for the late reply. Made the changes in the new patch.

@0001vrn
Copy link

0001vrn commented Dec 7, 2019

Hey,
Sorry for the late reply. Made the changes in the new patch.

Please address all the review comments @GajeshS

def __init__(self):
self.start = trienode('')
def insert(self,id,data): # returns true on successful insertion (data == password)
temp = self.start
Copy link

Choose a reason for hiding this comment

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

The variable name is still temp. Please change it to something meaningful @GajeshS

Copy link

@adhithya-srinivasan adhithya-srinivasan left a comment

Choose a reason for hiding this comment

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

Rename temp

@abranhe abranhe merged commit bcf7e28 into AllAlgorithms:master Oct 3, 2020
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.

4 participants