-
-
Notifications
You must be signed in to change notification settings - Fork 137
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
Re-design robot-simulator
#783
Comments
DirectionsReading https://github.com/exercism/problem-specifications/blob/main/exercises/robot-simulator/canonical-data.json I think we should go with the If the canonical data would be divised in a list of instruction: Accessing stateI think the most common here is to have a Having another object to represent both position and direction could be interesting, we could use it in construct and in a getter. But I think this would add unnecessary complexity. ConclusionI would vote for the following interface: class RobotSimulator {
public function __construct(private array $position, private string $direction) {}
public function instructions(string $instructions): void {}
public function getPosition(): array {}
public function getDirection(): string {}
} The student still is free to declare the enum he want for increased typing / validation. |
Thanks for your feedback. I'm totally OK with getters and I won't add Also I'll include it all in #784 together with the missing type specifications requested. @homersimpsons If you have no time for reviewing again: I'll merge it on Monday then. |
Perfect let's go vith this then.
I'm okay with this
I will have time to review it today, If you do this accross the week-end I won't reall be available but could try to get a glance. In anycase I'm okay if you merge it on monday even without review. |
The current exercise design has many problems:
position
anddirection
properties directly, butprotected
fields are provided in the stubRobot
vs.RobotSimulator.php
The tests expect methods to move the robot, while problem spec only has(will be solved in Sync robot-simulator #784)instructions
The tests are clumped together with no clear separation between test casesProblem specifications do not ask for a certain implementation. They define the problem statement as open as possible. Our design also should allow as many solutions as possible.
Current stats:
No discussion about these changes
instructions()
only or fluent interface only. (see Sync robot-simulator #784)Directions
Suggestion 1: Strings only
This leaves all possibilities open. Mentors can suggest
enum
or such to students. Butenum
is optimal for interfaces, internalenum
usage just complicates things.Suggestion 2: Provide
enum
stubIn addition to the tests from problem specifications we test the
enum
implementation first. When that is implemented, the actual problem is tested for. In these tests we must use theenum
type, too (like the constants now).This forces students to use
enum
. It raises the bar for students not familiar with that. Butenum
would be a good choice in a real life scenario.Suggestion 3: Provide ready-made
enum
As before, this forces students to use an
enum
type. But the level of required knowledge is lower (only usage is required). But still, it makes no sense to work with other types internally then.Which way to go? Other suggestions?
Accessing state
I think testing the state by accessing fields was done to force students to implement a state encapsulating solution with
__get()
or such. The exercise is marked astopic: ['oop']
inconfig.json
, which hints on why it may have been designed like this.To stay as close to the problem spec as possible, the OOP-isms are seemingly superfluous. Accessing protected properties via
__get()
could be avoided OOP-style also.Suggestion 1:
public
propertiesThis is closest to the problem specification and the easiest to implement. But it violates fundamental rules of OOP - hide the details behind methods.
Suggestion 2:
get
methodsAsk to implement getter methods to access the internal state. This also is easy to do. It makes the properties readonly from the outside but mutable from inside.
Suggetsion 3:
state
methodCombine the internal state to an array or result object holding all internal values we are interested in. Looks a bit artificial, but we have that in another exercise (
Solution
) already and it is good OOP practice.The text was updated successfully, but these errors were encountered: