davorian
/
toyrobot
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Added FEEDBACK.md and initial modifications based on this.

This commit is contained in:
Chris Davoren 2023-10-21 17:10:27 +10:00
parent 094845f2a7
commit aff508d835
2 changed files with 34 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

34
FEEDBACK.md Normal file
View File

@ -0,0 +1,34 @@
# Feedback
Given 21/10/2023.
## Original Feedback
What was great:
1. A lot of comments, show excellent rigor. There are many people with many differing opinions on the value of comments, but I think in this instance it demonstrates a good professional practice.
1. It works.
1. Includes tests AND examples as well as a way to parse examples into the runtime of the robot. Nice!
What could have been better:
1. robot.set_limits doesn’t appear to be necessary, also doesn’t check that the mutation of the robot state does not invalidate the robot placement.
2. robot.get_limits doesn’t appear to be necessary for the purposes of the exercise, and is currently unused.
3. Movement vectors is a good design decision imo. I think I did the same thing, it converts to semantics of direction to board placement.
4. Good use of specifications to return from an invalid request on movement
5. Rotation functions seems sensible and use modulus function to cycle within the domain of directions.
6. The interpret_command function seems sensible, although the specifications do not require cases insensitivity. Another oddity is that it appears it is only when an invalid PLACE command it attempted, the user is presented with text for valid commands, rather than a default case for the command switch statement. The requirement don’t include informing users of valid commands.
7. The string representation of the robot state, use for describing its position should probably not return “uninitialised” if there is no board state, I believe that this is addressed in the requirements.
8. The test structure could use a little more rigor. BDD and AAA practices are pretty universal. One of the main problems of a test is communicating what functionality you are proving, and often we want to evaluate that against relevant requirement specifications. Naming which communicates which requirement it is proving is very helpful.
9. The examples and the tests execute as expected but there is not implementation of the main program loop. Part of the requirements is to take user input.
## Initial Response/Notes
1. Fair, and also a logical error/omission on my part. These functions were added with the mindset of more complete get/setter coverage, but I agree they're not needed for this problem. Removed.
2. As for (1)
3. N/A (but thanks)
4. N/A
5. N/A
6. Case sensitivity *was* implied by the examples, but it was not explicitly clarified one way or the other. Case insensivity was easy to implement and seemed to represent the most "open" interpretation of the requirements, or so I thought.
7. Agree that if one considers a "REPORT" command to be "invalid" if the board is uninitialized, then it should simply be discarded as per the requirements. I'm willing to be persuaded otherwise, but I believe my line of thinking at the time was that REPORT is not an invalid *command*, but called on an invalid *state*, so it should behave sensibly in that setting.
8. Totally fair, the test structure is definitely quite *ad hoc*. Honestly formalised testing is one of my weak areas, I suppose a consequence of most of my experience coming from a research context. I will look more closely at BDD and AAA, thank you.
9. It would be easy enough to implement this, although there is no interactive input loop mentioned in the requirements I was working off. The README shows the version of the requirements the solution is based off.

28
toyrobot/robot.py
View File

@ -79,34 +79,6 @@ class Robot:
self._position_y = None
self._direction = None
def set_limits(self, max_x: int, max_y: int):
"""
Sets the positional limits for this Robot. See __init__() for details.
Args:
max_x (int): New maximum allowable horizontal coordinate.
max_y (int): New maximum allowable vertical coordinate.
Raises:
ValueError: If max_x or max_y are less than 0.
"""
if max_x < 0 or max_y < 0:
raise ValueError("Cannot specify negative limits")
self._max_x = max_x
self._max_y = max_y
def get_limits(self) -> (int, int):
"""
Returns the current maximum coordinates valid for this instance.
Returns:
(int, int): A tuple of the maximum coordinates in the order
(maximum_x, maximum y).
"""
return (self._max_x, self._max_y)
def valid_position(self, x: int, y: int) -> bool:
"""
Calculates whether the given coordinates are valid for the limits set