trie class and insert complete with all tests passing - #24
Conversation
| start = self.root | ||
| for letter in token: | ||
| start = start.setdefault(letter, OrderedDict()) | ||
| start['#'] = '#' |
There was a problem hiding this comment.
Since you're using '#' as your end of word token throughout your code, it would be a great idea and correct python usage to set a global constant e.g. TERM_CHAR = '#'
| return False | ||
| counter += 1 | ||
| try: | ||
| return start['#'] == '#' |
There was a problem hiding this comment.
This seems out of place here. It would be more explicit and assertive to do this check only once you've reached the very end of your token, so you check for sure once you exit the loop that the end-of-word char is sitting there.
Otherwise it seems you're taking an approach of "I know this is gonna work so I'll just leave it in there."
This will fail in a certain case. Suppose I insert the word "Python" into the tree. When you call contains("Pythonista"), it will return True though I have not inserted that word.
On that note, also test for a scenario like the above to make sure contains("Py") returns False.
| pending_list.append((k, v)) | ||
|
|
||
| def traversal(self, start=None): | ||
| import pdb; pdb.set_trace() |
There was a problem hiding this comment.
We got a PDB on the loose here!
| def test_contain_false_1(): | ||
| """Test if a trie contains a word.""" | ||
| trie = Trie(['cow', 'coward', 'company']) | ||
| assert not trie.contains('co') |
There was a problem hiding this comment.
This is a good test; interestingly, it is failing for many of the test cases I have made. Usually with longer strings. Try this when instantiating the Trie empty then pushing in each item. Also try with much longer strings e.g. "this_long_string" then check contains("this_long_strin")
| try: | ||
| return start['#'] == '#' | ||
| except KeyError: | ||
| pass |
There was a problem hiding this comment.
I spent some more time analyzing your algorithm. It has a problem where it incorrectly returns True when you check contains() on a stub of an inserted item.
e.g. when you insert "Python", contains("Py") should return False, but you're getting True.
I identified this line as the issue. Consider it very closely. What scenario is leading to this line being hit? What should you do instead?
| if not start: | ||
| raise KeyError('Start point not found') | ||
| pending_list = [] | ||
| for k, v in reversed(start.items()): |
There was a problem hiding this comment.
What's the idea here with reversing the keys and values? They are in arbitrary order in a dict, so it shouldn't make a difference.
Edit: I forgot that you're using an OrderedDict. Still though, this is a costly operation which shouldn't be needed.
| for k, v in cur[1].items(): | ||
| pending_list.append((k, v)) | ||
|
|
||
| def traversal(self, start=None): |
There was a problem hiding this comment.
I'll tell you guys now that you are over-complicating this traverse. Even though you're using an OrderedDict, it's OK it the traversal returns items in an arbitrary order.
Consider a recursive approach where you are simply concatenating together longer and longer strings, and finally yielding them whenever you identify that there is an end-of-token char present,
3 participants
No description provided.