-
Notifications
You must be signed in to change notification settings - Fork 77
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
Roza-cedar #62
base: master
Are you sure you want to change the base?
Roza-cedar #62
Conversation
anagram, frequency functions
Grabbing this to grade! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty good...
I feel that your anagrams solution has a lot of issues (see the comments) that I'm a bit confused by as you seem to understand how to use dictionaries to solve the other problems. As such, I am marking this Yellow to encourage you to take another look at that part.
Additionally there's some very vague names as well as very repetitive code in your sudoku solution that should usually be seen as a sign that you can refactor it to avoid the repetition. There are also missing complexity calculations for the anagrams and sudoku solutions.
Nonetheless, your top-K solution looks fine.
Regardless, I do encourage you to think about the anagrams solution more so this is a Yellow.
if len(strings) == 1: | ||
return [[strings[0]]] | ||
list1 = [strings[0]] | ||
listOfLists = [list1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are rather generic names that more describe what the data type is rather than what it is for. You don't even need list1
because you can just do [[strings[0]]]
, and something like return_value
or the like would be more informative than listOfLists
.
list1 = [strings[0]] | ||
listOfLists = [list1] | ||
for word in strings[1:]: | ||
flag = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, this could be better named something like matching_list_found
rather than flag
.
flag = False | ||
for lists in listOfLists: | ||
# check in the existing inner lists | ||
if anagram_helper(word) == anagram_helper(lists[0]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This solution redoes a great deal of work as it recalculates the frequency map through anagram_helper
again and again for each word in the list.
Additionally going back over every list makes this algorithm balloon to O(n^2) in time complexity since in the worst case, every word is in its own list and there are none that share anagrams.
I think you can achieve a much better solution that is O(n) through using a dictionary, because with this solution you don't gain advantage of the O(1) lookup time of dictionaries.
@@ -5,17 +5,56 @@ def grouped_anagrams(strings): | |||
Time Complexity: ? | |||
Space Complexity: ? | |||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling out the missing complexity calculations here, but I haven't been dinging people for it.
if table[i][j] in seen_dict: | ||
return False | ||
else: | ||
seen_dict[table[i][j]] = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works, but usually when you repeat almost the same code so many times, it's usually worth thinking about how you can cut down on duplication through perhaps defining a helper function.
Code repetition like this tends to lead to errors and higher maintenance as the different versions of almost the same code fall out of sync with each other.
@@ -25,5 +64,119 @@ def valid_sudoku(table): | |||
Time Complexity: ? | |||
Space Complexity: ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, complexity calculations are missing here.
anagram, frequency functions
Hash Table Practice
Congratulations! You're submitting your assignment!
Comprehension Questions