feature(C08_P06): added oops solution #225
Conversation
|
https://github.com/careercup/CtCI-6th-Edition-Python/runs/6045332210?check_suite_focus=true |
|
Also to directly answer your question, the decorator might work if you add @staticmethod to it. not sure. |
Tried this. Was getting some errors. Went ahead with your examples (Option B). Thanks for the detailed review. Really learnt a lot here. |
|
I see these extra commits got added. Probably as you attempted to rebase, which can be tricky to get right. I'll just delete them when this gets merged. |
chapter_08/p06_towers_of_hanoi.py
Outdated
| def pop(self): | ||
| if len(self._stack): | ||
| return self._stack.pop() | ||
| raise Exception("Stack Empty") |
There was a problem hiding this comment.
If you just
def pop(self):
return self._stack.pop()then it would return a more properly typed error message. If it was me I'd just leave that default error (IndexError).
If you do want to return your own custom error then I'd suggest following the EAFP coding style that is preferred in python.
def pop(self):
try:
return self._stack.pop()
except IndexError:
raise IndexError("pop from an empty stack")There was a problem hiding this comment.
got it. adding this pattern for the Stack class exceptions
chapter_08/p06_towers_of_hanoi.py
Outdated
|
|
||
| def push(self, val): | ||
| if len(self._stack) == self.stack_size: | ||
| return |
There was a problem hiding this comment.
Somewhat subjective but since pop throws an exception if the stack is too small it feels like this should throw an exception if the stack is too big.
class StackTooBigError(Exception):
pass
def push(self, val):
if len(self._stack) == self.stack_size:
raise StackTooBigError("stack has reached max size")There was a problem hiding this comment.
Makes sense. Implemented.
chapter_08/p06_towers_of_hanoi.py
Outdated
|
|
||
| def get_stack(self, stack_num): | ||
| if 0 > stack_num >= self.num_stacks: | ||
| raise Exception("stack_num invalid") |
There was a problem hiding this comment.
In general one should avoid raising generic Exceptions. IndexError would probably be a better fit here.
There was a problem hiding this comment.
I agree. Implemented.
chapter_08/p06_towers_of_hanoi.py
Outdated
| return self._stack[-1] | ||
| raise Exception("Stack Empty") | ||
|
|
||
| def is_empty(self): |
There was a problem hiding this comment.
is_empty is not used so I'd suggest deleting it.
chapter_08/p06_towers_of_hanoi.py
Outdated
| raise Exception("Stack Empty") | ||
|
|
||
| def top(self): | ||
| if len(self._stack): |
There was a problem hiding this comment.
same comment as previous.
|
Addressed all the above comments. Thanks again for the detailed review! Edit: Yes those additional commits came in because of the master rebase. |
|
I merged your work but forgot to run the linter beforehand. I'll fix those now |
Added a more structured oops based approach. No modification in logic.