Solution with comments (see "unknown_alphabet" function) #28
Conversation
SilverSolver
requested review from
Sridha2412,
crossopt,
evinpinar and
randomUsernameAndPassword
| @@ -0,0 +1,49 @@ | |||
| from collections import defaultdict | |||
|
|
|||
| def dfs(ordered_vertices, edges, color, v): | |||
There was a problem hiding this comment.
This dfs is seriously scary.
I think it needs at least some comments. What does it return and why?
My current guess is topsort order and a color array if it exists or None if the graph has a cycle, but why return them if the function modifies them anyway?
Also imho searching for cycles and topsorting in the same function causes unnecessary confusion. Maybe comments would help, but why not split this stuff into two functions anyway?
There was a problem hiding this comment.
Oh I think we should look for cycles during the topsort, because they both require searching on graph. I liked the coloring, which helps detecting if it is possible or not, but not sure how to use it for finding maximal satisfying subset.
Also, comments would be great indeed.
There was a problem hiding this comment.
Ok, I'll add comments soon
| N = len(symbols) | ||
| color = dict() | ||
| for v in symbols: | ||
| color[v] = 0 |
There was a problem hiding this comment.
stuff like this could look cleaner as
color = dict.fromkeys(symbols, 0)
| for w1, w2 in zip(dictionary[:-1], dictionary[1:]): | ||
| for i in range(min(len(w1), len(w2))): | ||
| symbols.add(w1[i]) | ||
| if w1[i] != w2[i]: |
There was a problem hiding this comment.
Once again, I do get the feeling that there's a lot going on in these functions.
Possibly splitting up some of the logic could help?
There was a problem hiding this comment.
I added some comments here. Hope it will be more clear now.
| ordered_vertices.extend(residual_symbols) | ||
| return ordered_vertices[::-1] | ||
|
|
||
| def unknown_alphabet(dictionary): |
There was a problem hiding this comment.
I think the name of the function is not definitive. Something like "find_alphabet" might be better?
| self.assertFalse(unknown_alphabet(["1", "2", "3", "2"])) | ||
|
|
||
| def test_2(self): | ||
| self.assertEqual(unknown_alphabet(["1a", "1b", "a1", "ab"]), ["1", "a", "b"]) |
There was a problem hiding this comment.
Nice tests, we should merge them with @crossopt 's unittests.
There was a problem hiding this comment.
Yes, I've already think about it
| @@ -0,0 +1,49 @@ | |||
| from collections import defaultdict | |||
|
|
|||
| def dfs(ordered_vertices, edges, color, v): | |||
There was a problem hiding this comment.
Oh I think we should look for cycles during the topsort, because they both require searching on graph. I liked the coloring, which helps detecting if it is possible or not, but not sure how to use it for finding maximal satisfying subset.
Also, comments would be great indeed.
SilverSolver
changed the title
SilverSolver
changed the title
Initial solution and some tests!