binary_tree_traversals.py: Simplify with dataclasses - #4336
Conversation
ghost
added
enhancement
dhruvmanila
left a comment
dhruvmanila
left a comment
There was a problem hiding this comment.
Awesome work!
This is out of scope for this PR but just wanted to point out that I see a lot of Node classes containing a left and right node. I don't think that makes sense.
In case of tree structures a node can only contain key and data, key to identify the node and data is the containing information. The tree has left and right subtrees and not the node.
ghost
added
awaiting changes
Co-authored-by: Dhruv Manilawala <dhruvmanila@gmail.com>
ghost
added
awaiting reviews
There was a problem hiding this comment.
I did not notice this the first time but shouldn't this be Optional["Node"]?
There was a problem hiding this comment.
My sense was that if neither Python nor mypy was complaining about it then I would not either. This would be the only location where we would set these values to None unless we started deleting Nodes which I assumed we were not gonna do.
There was a problem hiding this comment.
Python is never going to complain as it simply does not care about type hints. mypy will complain when you run it in a bit stricter modes. Plain mypy will not complain about a lot of stuff as that's how gradual typing goes.
And no, as per this definition, the leaves of the trees (the last nodes) will have its left and right node None as there are no left and right child (we have reached the end).
There was a problem hiding this comment.
This would be the only location where we would set these values to None
There was a problem hiding this comment.
Yes, but that still won't allow the file to be type checked later on. The variable's correct type is Optional["Node"]. Just because it is being set None once, does not change the type of that variable. That's what 'static' means in static typing :)
2 participants
Describe your change:
Checklist:
Fixes: #{$ISSUE_NO}.