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

Implement Robot Simulator #146

Merged
merged 1 commit into from Jul 7, 2016

Conversation

Projects
None yet
6 participants
@IanWhitney
Contributor

IanWhitney commented Jun 27, 2016

Implement Robot Simulator

This follows the current standard test suite

The stub file, as we discussed in #117, is just enough to run cargo test and not get any errors from ignored tests.

Show outdated Hide outdated exercises/robot-simulator/src/lib.rs
Robot {}
}
pub fn turn_right(&mut self) {}

This comment has been minimized.

@jonasbb

jonasbb Jun 27, 2016

Contributor

I would pre-fill the functions with unimplemented!() just to make sure, that you know where to put the implementation. This is more important for functions which return values, because unimplemented!() always typechecks, no matter the type.

@jonasbb

jonasbb Jun 27, 2016

Contributor

I would pre-fill the functions with unimplemented!() just to make sure, that you know where to put the implementation. This is more important for functions which return values, because unimplemented!() always typechecks, no matter the type.

This comment has been minimized.

@IanWhitney

IanWhitney Jun 27, 2016

Contributor

Cool. Done. Thanks!

@IanWhitney

IanWhitney Jun 27, 2016

Contributor

Cool. Done. Thanks!

Show outdated Hide outdated exercises/robot-simulator/src/lib.rs
West,
}
pub struct Robot {

This comment has been minimized.

@jonasbb

jonasbb Jun 27, 2016

Contributor

I would personally use pub struct Robot;, because it feels a bit less pushed towards "normal" structs, because a tuple struct like Robot(Direction, i32, i32) would also be possible.

@jonasbb

jonasbb Jun 27, 2016

Contributor

I would personally use pub struct Robot;, because it feels a bit less pushed towards "normal" structs, because a tuple struct like Robot(Direction, i32, i32) would also be possible.

@@ -0,0 +1,32 @@
#[derive(PartialEq, Debug)]

This comment has been minimized.

@jonasbb

jonasbb Jun 27, 2016

Contributor

Is there a reason to not derive Eq as well? Maybe, also Copy and Clone, which makes working with references easier.

@jonasbb

jonasbb Jun 27, 2016

Contributor

Is there a reason to not derive Eq as well? Maybe, also Copy and Clone, which makes working with references easier.

This comment has been minimized.

@IanWhitney

IanWhitney Jun 27, 2016

Contributor

I was only adding what I needed in order to get the compiler to compile. I imagine the real solution will have more derived traits.

@IanWhitney

IanWhitney Jun 27, 2016

Contributor

I was only adding what I needed in order to get the compiler to compile. I imagine the real solution will have more derived traits.

This comment has been minimized.

@petertseng

petertseng Jul 3, 2016

Member

The takeaway I get from this is: "If students think that they can't add more traits, we should add a comment telling them they can". I don't know whether the conditional is true (I can't read minds).

@petertseng

petertseng Jul 3, 2016

Member

The takeaway I get from this is: "If students think that they can't add more traits, we should add a comment telling them they can". I don't know whether the conditional is true (I can't read minds).

This comment has been minimized.

@IanWhitney

IanWhitney Jul 3, 2016

Contributor

For my example code I ended up not needing to derive Eq or Copy. But I did need Clone. Other students may need different traits, though.

What we might want at the top of all of our stub files is a general comment to say "This file is here to get you started. Change/add/remove code as you see fit."

@IanWhitney

IanWhitney Jul 3, 2016

Contributor

For my example code I ended up not needing to derive Eq or Copy. But I did need Clone. Other students may need different traits, though.

What we might want at the top of all of our stub files is a general comment to say "This file is here to get you started. Change/add/remove code as you see fit."

This comment has been minimized.

@IanWhitney

IanWhitney Jul 3, 2016

Contributor

Comment added.

@IanWhitney

IanWhitney Jul 3, 2016

Contributor

Comment added.

This comment has been minimized.

@petertseng

petertseng Jul 3, 2016

Member

Seems good. It makes me wonder that if we are going to be in the habit of providing stubs, maybe this comment should be a standard that goes on top of all of them...

clearly an idea for another PR, but neverthelss worth bringing up while we are gthinking about it here

@petertseng

petertseng Jul 3, 2016

Member

Seems good. It makes me wonder that if we are going to be in the habit of providing stubs, maybe this comment should be a standard that goes on top of all of them...

clearly an idea for another PR, but neverthelss worth bringing up while we are gthinking about it here

@jonasbb

This comment has been minimized.

Show comment
Hide comment
@jonasbb

jonasbb Jun 27, 2016

Contributor

Your test suite will push them to a certain solution in any case. If you set mut it is more likely to change the state of the struct, but if you remove mut and return a new struct, you need to adapt your test cases to support this.

Contributor

jonasbb commented Jun 27, 2016

Your test suite will push them to a certain solution in any case. If you set mut it is more likely to change the state of the struct, but if you remove mut and return a new struct, you need to adapt your test cases to support this.

IanWhitney pushed a commit to IanWhitney/xrust that referenced this pull request Jun 27, 2016

Ian Whitney
Simplify struct
As suggested by jonasbb

exercism#146 (comment)
@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Jun 27, 2016

Contributor

Ah, right, if I remove the mut then the signature would change to

fn turn_right(&self) -> Self {
  Robot::New{...}
}

Which might be interesting. I don't think any of our exercises take that approach.

Contributor

IanWhitney commented Jun 27, 2016

Ah, right, if I remove the mut then the signature would change to

fn turn_right(&self) -> Self {
  Robot::New{...}
}

Which might be interesting. I don't think any of our exercises take that approach.

IanWhitney pushed a commit to IanWhitney/xrust that referenced this pull request Jun 29, 2016

Ian Whitney
Update stub push towards immutable robots
As mentioned here:
exercism#146 (comment)

I like the idea of pushing towards a solution with immutable robots.
It's a different way of using structs than our other exercises do. As a
side benefit, it shortens the test functions.

Students can always edit the test file or the stub file, if they so
wish. So they could still implement mutable robots, though I don't think
many (or any, probably) will do so.
@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Jun 29, 2016

Contributor

Two tweaks:

  • Stub & Tests now use immutable robots.
  • I'm silencing some warnings
Contributor

IanWhitney commented Jun 29, 2016

Two tweaks:

  • Stub & Tests now use immutable robots.
  • I'm silencing some warnings
@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Jun 29, 2016

Contributor

I'm unsure what the direction function should return -- Direction or &Direction?

Currently the Robot new function uses &Direction, which makes sense to me. Students can choose to handle the lifetimes that come with a struct using a borrowed reference, or they can derive Clone and just clone the Direction. That works for me.

But when getting the Direction back out, what is the idiomatic thing to do? My instinct says that the function should be

fn direction(&self) -> &Direction

But my instinct has been wrong a lot.

Contributor

IanWhitney commented Jun 29, 2016

I'm unsure what the direction function should return -- Direction or &Direction?

Currently the Robot new function uses &Direction, which makes sense to me. Students can choose to handle the lifetimes that come with a struct using a borrowed reference, or they can derive Clone and just clone the Direction. That works for me.

But when getting the Direction back out, what is the idiomatic thing to do? My instinct says that the function should be

fn direction(&self) -> &Direction

But my instinct has been wrong a lot.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Jun 29, 2016

Contributor

usually, accessor methods return &T. If they returned T, then they would destroy the owner, or you'd be making a clone every time.

Contributor

steveklabnik commented Jun 29, 2016

usually, accessor methods return &T. If they returned T, then they would destroy the owner, or you'd be making a clone every time.

@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Jun 29, 2016

Contributor

Ooh, my instinct was right for once. Go team instinct! 👏

Contributor

IanWhitney commented Jun 29, 2016

Ooh, my instinct was right for once. Go team instinct! 👏

@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Jun 29, 2016

Contributor

What about primitives, @steveklabnik ?

For example, given the struct

struct Robot {
  x: isize,
  y: isize
}

Would you expect a position function to be:

fn postion(&self) -> (isize, isize)

or

fn postion(&self) -> (&isize, &isize)

Alternatively we could create a whole new Position type, like we did with queen attack

Contributor

IanWhitney commented Jun 29, 2016

What about primitives, @steveklabnik ?

For example, given the struct

struct Robot {
  x: isize,
  y: isize
}

Would you expect a position function to be:

fn postion(&self) -> (isize, isize)

or

fn postion(&self) -> (&isize, &isize)

Alternatively we could create a whole new Position type, like we did with queen attack

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Jun 29, 2016

Contributor

If the type is Copy, then they're basically the same, so yeah, returning T is fine. Consider http://doc.rust-lang.org/stable/collections/vec/struct.Vec.html#method.len as an example

Contributor

steveklabnik commented Jun 29, 2016

If the type is Copy, then they're basically the same, so yeah, returning T is fine. Consider http://doc.rust-lang.org/stable/collections/vec/struct.Vec.html#method.len as an example

@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Jun 29, 2016

Contributor

That's what I was thinking as well. I'm clearly on a roll today.

Contributor

IanWhitney commented Jun 29, 2016

That's what I was thinking as well. I'm clearly on a roll today.

IanWhitney pushed a commit to IanWhitney/xrust that referenced this pull request Jun 29, 2016

Ian Whitney
Direction accessor returns borrowed Direction
As discussed

exercism#146 (comment)

and

exercism#146 (comment)

Per Steve Klabnik

> usually, accessor methods return &T. If they returned T, then they
> would destroy the owner, or you'd be making a clone every time
@Dr-Emann

This comment has been minimized.

Show comment
Hide comment
@Dr-Emann

Dr-Emann Jun 29, 2016

Taking by value and returning a copy just doesn't feel rusty-y to me. I expect functions to take the most general type of arguments they can, &T if no mutation, &mut T if mutation needed, and T only if ownership is required.

Taking by value and returning a copy just doesn't feel rusty-y to me. I expect functions to take the most general type of arguments they can, &T if no mutation, &mut T if mutation needed, and T only if ownership is required.

@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Jun 29, 2016

Contributor

Working example in place. Now to figure out where to place this problem.

The solution will involve:

  • Enums (though the stub file provides them)
  • Structs (either a tuple struct, or a normal struct)
  • Some matching on Direction variant
  • Iteration

And, if the student chooses not to clone the Direction, they may end up in a world of Lifetimes. That, though, is optional.

I'm thinking it should go around grade-school. It's got a pretty straightforward use of enums, so it should come before Allergies (which has slightly more complex enum usage). But beyond enums, students will be familiar with all the tools they need to solve this by the time they reach grade-school

Contributor

IanWhitney commented Jun 29, 2016

Working example in place. Now to figure out where to place this problem.

The solution will involve:

  • Enums (though the stub file provides them)
  • Structs (either a tuple struct, or a normal struct)
  • Some matching on Direction variant
  • Iteration

And, if the student chooses not to clone the Direction, they may end up in a world of Lifetimes. That, though, is optional.

I'm thinking it should go around grade-school. It's got a pretty straightforward use of enums, so it should come before Allergies (which has slightly more complex enum usage). But beyond enums, students will be familiar with all the tools they need to solve this by the time they reach grade-school

@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Jun 29, 2016

Contributor

So what API are you proposing, @Dr-Emann?

Contributor

IanWhitney commented Jun 29, 2016

So what API are you proposing, @Dr-Emann?

@Dr-Emann

This comment has been minimized.

Show comment
Hide comment
@Dr-Emann

Dr-Emann Jun 30, 2016

I would expect turn_right, turn_left, advance, and instructions to take &mut self, instead of self, and return nothing.

I would expect turn_right, turn_left, advance, and instructions to take &mut self, instead of self, and return nothing.

@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Jun 30, 2016

Contributor

That was the initial implementation. And it might be the right one. But I'm leaning towards immutable because

  • Immutability is the default in Rust
  • No other problem (that I know of) really features Rust's default immutability
  • Immutability is not my natural inclination

I figure if I don't expect immutability then other programmers with an OO-focused background probably don't expect immutability. So a problem that forces immutability may make me (and them) think about immutability. Which I think is good.

Immutability brings other benefits, though they aren't exposed by the tests. It would be very easy to trace your Robot's path, for example. Re-winding to a specific point on the path would be trivial. All very nice, but not part of the test suite.

Contributor

IanWhitney commented Jun 30, 2016

That was the initial implementation. And it might be the right one. But I'm leaning towards immutable because

  • Immutability is the default in Rust
  • No other problem (that I know of) really features Rust's default immutability
  • Immutability is not my natural inclination

I figure if I don't expect immutability then other programmers with an OO-focused background probably don't expect immutability. So a problem that forces immutability may make me (and them) think about immutability. Which I think is good.

Immutability brings other benefits, though they aren't exposed by the tests. It would be very easy to trace your Robot's path, for example. Re-winding to a specific point on the path would be trivial. All very nice, but not part of the test suite.

IanWhitney pushed a commit to IanWhitney/xrust that referenced this pull request Jun 30, 2016

Ian Whitney
Add robot-simulator to exercise track
As per:
exercism#146 (comment)

> The solution will involve:
>
>   Enums (though the stub file provides them)
>   Structs (either a tuple struct, or a normal struct)
>   Some matching on Direction variant
>   Iteration
>
> And, if the student chooses not to clone the Direction, they may end up
> in a world of Lifetimes. That, though, is optional.
>
> I'm thinking it should go around grade-school. It's got a pretty
> straightforward use of enums, so it should come before Allergies (which
> has slightly more complex enum usage). But beyond enums, students will
> be familiar with all the tools they need to solve this by the time they
> reach grade-school
@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Jun 30, 2016

Contributor

Ok. This now passes CI and is basically ready to go. Once we settle on API and improve the example code a bit, it'll be ready to merge.

Contributor

IanWhitney commented Jun 30, 2016

Ok. This now passes CI and is basically ready to go. Once we settle on API and improve the example code a bit, it'll be ready to merge.

@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Jun 30, 2016

Contributor

Thinking about the immutability thing more...

As I mentioned earlier, I think the immutable design advantages become clear if you want to rewind to a point. Maybe we could have an optional test that's commented out:

// Optional:
// #[test]
// fn robot_simulator_can_return_robot_at_any_point() {
// let robot = Robot::new(...);
// let simulator = RobotSimulator::new(robot);
// simulator.simulate("LAAAR");
// assert_eq!((-1,2), simulator.at_step(4).robot_position);
// assert_eq!((0,2), simulator.at_step(3).robot_position);
// etc.
// }
Contributor

IanWhitney commented Jun 30, 2016

Thinking about the immutability thing more...

As I mentioned earlier, I think the immutable design advantages become clear if you want to rewind to a point. Maybe we could have an optional test that's commented out:

// Optional:
// #[test]
// fn robot_simulator_can_return_robot_at_any_point() {
// let robot = Robot::new(...);
// let simulator = RobotSimulator::new(robot);
// simulator.simulate("LAAAR");
// assert_eq!((-1,2), simulator.at_step(4).robot_position);
// assert_eq!((0,2), simulator.at_step(3).robot_position);
// etc.
// }

@IanWhitney IanWhitney changed the title from WIP: Robot Simulator tests & stub file. Do not merge. to Implement Robot Simulator Jul 1, 2016

@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Jul 1, 2016

Contributor

I think this is now ready. Leaving the PR open until Tuesday, July 5th, after which I'll merge if there are no issues.

Contributor

IanWhitney commented Jul 1, 2016

I think this is now ready. Leaving the PR open until Tuesday, July 5th, after which I'll merge if there are no issues.

@Ryman

This comment has been minimized.

Show comment
Hide comment
@Ryman

Ryman Jul 1, 2016

@IanWhitney Is the intention for the example implementation to be showing idiomatic rust, or just a potential solution to pass the tests?

Ryman commented Jul 1, 2016

@IanWhitney Is the intention for the example implementation to be showing idiomatic rust, or just a potential solution to pass the tests?

@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Jul 1, 2016

Contributor

@Ryman That is a good question without a clear answer!

Track maintainers discussed this recently: exercism/discussions#27

Unsurprisingly, people had different opinions. I think everyone is agreed that the first job of the example code is to pass the tests (otherwise our CI fails).

Beyond that first requirement, there was general agreement (though not unanimous) that the code should be:

  • not clever
  • idiomatic
  • a good example

Probably my favorite description of the example code was this one:

[T]ry to solve the new proposed exercise as if the track maintainer were a real exercism user.

Which I most certainly am. I ended up writing Rust exercises because I wanted to use Exercism to help me learn Rust. My example code for this exercise is probably pretty close to what I would submit if I did this exercise.

Contributor

IanWhitney commented Jul 1, 2016

@Ryman That is a good question without a clear answer!

Track maintainers discussed this recently: exercism/discussions#27

Unsurprisingly, people had different opinions. I think everyone is agreed that the first job of the example code is to pass the tests (otherwise our CI fails).

Beyond that first requirement, there was general agreement (though not unanimous) that the code should be:

  • not clever
  • idiomatic
  • a good example

Probably my favorite description of the example code was this one:

[T]ry to solve the new proposed exercise as if the track maintainer were a real exercism user.

Which I most certainly am. I ended up writing Rust exercises because I wanted to use Exercism to help me learn Rust. My example code for this exercise is probably pretty close to what I would submit if I did this exercise.

@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Jul 1, 2016

Contributor

That's not to say my example code is exactly idiomatic, though. I'm still quite new to Rust and am learning new stuff all the time. Which is one of the reasons I encourage discussion in these PRs.

Contributor

IanWhitney commented Jul 1, 2016

That's not to say my example code is exactly idiomatic, though. I'm still quite new to Rust and am learning new stuff all the time. Which is one of the reasons I encourage discussion in these PRs.

Show outdated Hide outdated exercises/robot-simulator/example.rs
}
}
pub fn turn_right(self) -> Self {

This comment has been minimized.

@petertseng

petertseng Jul 2, 2016

Member

I'll not comment on whether we want to go with mutable or immutable interface.

But I want to say that if we want an immutable interface, I suspect we can do turn_right(&self) -> Self instead of turn_right(self) -> Self. Can you check?

(And if it were mutable, it would be turn_right(&mut self))

@petertseng

petertseng Jul 2, 2016

Member

I'll not comment on whether we want to go with mutable or immutable interface.

But I want to say that if we want an immutable interface, I suspect we can do turn_right(&self) -> Self instead of turn_right(self) -> Self. Can you check?

(And if it were mutable, it would be turn_right(&mut self))

This comment has been minimized.

@IanWhitney

IanWhitney Jul 3, 2016

Contributor

That results in compiler errors. Can not move out of borrowed content. There are additional changes we'd need to make to get that to work, though I don't know what they are off the top of my head.

@IanWhitney

IanWhitney Jul 3, 2016

Contributor

That results in compiler errors. Can not move out of borrowed content. There are additional changes we'd need to make to get that to work, though I don't know what they are off the top of my head.

This comment has been minimized.

@petertseng

petertseng Jul 3, 2016

Member

This comment surprised me enough that I checked out your branch to take a look.

My diagnosis: Make your Position #[derive(Clone, Copy)] and it will work.

@petertseng

petertseng Jul 3, 2016

Member

This comment surprised me enough that I checked out your branch to take a look.

My diagnosis: Make your Position #[derive(Clone, Copy)] and it will work.

This comment has been minimized.

@petertseng

petertseng Jul 3, 2016

Member

same with advance and instructions - If your Direction is Copy then they can take &self

Can't do the same for execute though since it can potentially return self, which means a &self wouldn't be able to become a Self.

@petertseng

petertseng Jul 3, 2016

Member

same with advance and instructions - If your Direction is Copy then they can take &self

Can't do the same for execute though since it can potentially return self, which means a &self wouldn't be able to become a Self.

This comment has been minimized.

@IanWhitney

IanWhitney Jul 3, 2016

Contributor

Huh. I never would have guessed that from the compiler error. Clearly.

Based on this quote from The Book, I'll update these to take &self

We should default to using &self, as you should prefer borrowing over taking ownership, as well as taking immutable references over mutable ones.

@IanWhitney

IanWhitney Jul 3, 2016

Contributor

Huh. I never would have guessed that from the compiler error. Clearly.

Based on this quote from The Book, I'll update these to take &self

We should default to using &self, as you should prefer borrowing over taking ownership, as well as taking immutable references over mutable ones.

This comment has been minimized.

@petertseng

petertseng Jul 3, 2016

Member

well, to be fair, making them Copy was not the only solution - could have only made them Clone, not Copy, and then explicitly call clone() on them. But I suggested Copy simply because https://doc.rust-lang.org/std/marker/trait.Copy.html said "Generally speaking, if your type can implement Copy, it should"

@petertseng

petertseng Jul 3, 2016

Member

well, to be fair, making them Copy was not the only solution - could have only made them Clone, not Copy, and then explicitly call clone() on them. But I suggested Copy simply because https://doc.rust-lang.org/std/marker/trait.Copy.html said "Generally speaking, if your type can implement Copy, it should"

This comment has been minimized.

@petertseng

petertseng Jul 3, 2016

Member

I don't always do this myself (maybe I should if I run into a compiler output I don't understand), but rustc should have outputted src/lib.rs:67:21: 67:25 help: runrustc --explain E0507to see a detailed explanation

And I found that one of the solutions rustc --explain E0507 does say you can make it Copy, so that could have been a way to understand.

@petertseng

petertseng Jul 3, 2016

Member

I don't always do this myself (maybe I should if I run into a compiler output I don't understand), but rustc should have outputted src/lib.rs:67:21: 67:25 help: runrustc --explain E0507to see a detailed explanation

And I found that one of the solutions rustc --explain E0507 does say you can make it Copy, so that could have been a way to understand.

Show outdated Hide outdated exercises/robot-simulator/example.rs
pub fn instructions(self, instructions: &str) -> Self {
let mut r = Self::build(self.position, self.direction);
for c in instructions.chars() {

This comment has been minimized.

@petertseng

petertseng Jul 3, 2016

Member

Despite fold being possible here, this is straightforward. So I would probably stick with this, unless someone really thinnks we should use fold here.

@petertseng

petertseng Jul 3, 2016

Member

Despite fold being possible here, this is straightforward. So I would probably stick with this, unless someone really thinnks we should use fold here.

This comment has been minimized.

@IanWhitney

IanWhitney Jul 3, 2016

Contributor

I tried fold for a bit but was running into problems. Ownership, I think. This solution was my fallback. Once the problem is being solved by students I plan on checking their solutions to see how I should have been using fold.

That's what I do with most problems. Get it working myself, then review other solutions to figure out how I should be doing it.

@IanWhitney

IanWhitney Jul 3, 2016

Contributor

I tried fold for a bit but was running into problems. Ownership, I think. This solution was my fallback. Once the problem is being solved by students I plan on checking their solutions to see how I should have been using fold.

That's what I do with most problems. Get it working myself, then review other solutions to figure out how I should be doing it.

This comment has been minimized.

@IanWhitney

IanWhitney Jul 3, 2016

Contributor

Fold may have been failing because of the Clone/Copy problems you already diagnosed. I'll try it again.

@IanWhitney

IanWhitney Jul 3, 2016

Contributor

Fold may have been failing because of the Clone/Copy problems you already diagnosed. I'll try it again.

This comment has been minimized.

@petertseng

petertseng Jul 3, 2016

Member

well, looks like you did want to use fold, and well I'll not complain about you using it! I think I only hesitate because I'm not sure if the typical Rust user is comfortable thinking in folds, so I'm never sure whether it's idiomatic and whether I should just use a (relatively more) straightforward for instead. Maybe I should have more faith. It is true that it can result in shorter code.

@petertseng

petertseng Jul 3, 2016

Member

well, looks like you did want to use fold, and well I'll not complain about you using it! I think I only hesitate because I'm not sure if the typical Rust user is comfortable thinking in folds, so I'm never sure whether it's idiomatic and whether I should just use a (relatively more) straightforward for instead. Maybe I should have more faith. It is true that it can result in shorter code.

IanWhitney pushed a commit to IanWhitney/xrust that referenced this pull request Jul 3, 2016

Ian Whitney
Update function to borrow self, not take ownership
As @petertseng pointed out, I can update these functions to borrow
`self` if I give Direction and Position `Copy`

exercism#146 (comment)
Show outdated Hide outdated exercises/robot-simulator/example.rs
}
pub fn instructions(&self, instructions: &str) -> Self {
instructions.chars().fold(Self::build(self.position, self.direction),

This comment has been minimized.

@petertseng

petertseng Jul 3, 2016

Member

I haven't tested, but if you had Robot derive Clone, I think this Self::build(...) could just be clone() instead. But I don't see this as a change that has to be done.

@petertseng

petertseng Jul 3, 2016

Member

I haven't tested, but if you had Robot derive Clone, I think this Self::build(...) could just be clone() instead. But I don't see this as a change that has to be done.

@petertseng

This comment has been minimized.

Show comment
Hide comment
@petertseng

petertseng Jul 3, 2016

Member

For me, I think I've said all I want to say 👍

Member

petertseng commented Jul 3, 2016

For me, I think I've said all I want to say 👍

Show outdated Hide outdated exercises/robot-simulator/src/lib.rs
impl Robot {
#[allow(unused_variables)]
pub fn new(x: isize, y: isize, d: &Direction) -> Self {

This comment has been minimized.

@Ryman

Ryman Jul 5, 2016

The Direction argument being a reference here is a bit out of place when compared to the rest of the interface, for if we assume the user may want to store &Direction in their Robot type, the other methods will not allow this to be updated to anything reasonable, i.e.:

fn turn_right(&self) -> Self {
     Robot { 
         position: self.position,
         // nowhere to tie this lifetime to as it's not an input
         direction: &self.direction.next_clockwise() 
     }
}
@Ryman

Ryman Jul 5, 2016

The Direction argument being a reference here is a bit out of place when compared to the rest of the interface, for if we assume the user may want to store &Direction in their Robot type, the other methods will not allow this to be updated to anything reasonable, i.e.:

fn turn_right(&self) -> Self {
     Robot { 
         position: self.position,
         // nowhere to tie this lifetime to as it's not an input
         direction: &self.direction.next_clockwise() 
     }
}

This comment has been minimized.

@IanWhitney

IanWhitney Jul 6, 2016

Contributor

It's preferred for structs to take ownership of their values, so I'm OK with pushing students towards that.

@IanWhitney

IanWhitney Jul 6, 2016

Contributor

It's preferred for structs to take ownership of their values, so I'm OK with pushing students towards that.

This comment has been minimized.

@Ryman

Ryman Jul 6, 2016

To be clear, that means fn new(x: isize, y: isize, d: Direction) -> Self rather than the current signature with the push towards the student calling clone to take ownership?

@Ryman

Ryman Jul 6, 2016

To be clear, that means fn new(x: isize, y: isize, d: Direction) -> Self rather than the current signature with the push towards the student calling clone to take ownership?

unimplemented!()
}
pub fn direction(&self) -> &Direction {

This comment has been minimized.

@Ryman

Ryman Jul 5, 2016

Direction should probably be Copy which would make this idiomatically -> Direction

@Ryman

Ryman Jul 5, 2016

Direction should probably be Copy which would make this idiomatically -> Direction

This comment has been minimized.

@IanWhitney

IanWhitney Jul 6, 2016

Contributor

I don't think there's anything in the tests that forces Direction to be Copy. If there is, then I agree that this signature makes more sense. But if not, then I think we should stick with &T

@IanWhitney

IanWhitney Jul 6, 2016

Contributor

I don't think there's anything in the tests that forces Direction to be Copy. If there is, then I agree that this signature makes more sense. But if not, then I think we should stick with &T

Show outdated Hide outdated exercises/robot-simulator/tests/robot-simulator.rs
#[test]
#[ignore]
fn turning_left_from_north_points_the_robot_west() {
let robot = Robot::new(0, 0, &Direction::North).turn_left();

This comment has been minimized.

@Ryman

Ryman Jul 5, 2016

let robot = Robot::new(..);
let robot = robot.turn_left();
assert ...

If these tests were written in the above form then users might be able to implement the methods with either of these signatures:

  • fn builder_style(self) -> Self
  • fn with_copies(&self) -> Self
  • fn mutably(&mut self) -> &mut Self
@Ryman

Ryman Jul 5, 2016

let robot = Robot::new(..);
let robot = robot.turn_left();
assert ...

If these tests were written in the above form then users might be able to implement the methods with either of these signatures:

  • fn builder_style(self) -> Self
  • fn with_copies(&self) -> Self
  • fn mutably(&mut self) -> &mut Self

This comment has been minimized.

@IanWhitney

IanWhitney Jul 6, 2016

Contributor

In my testing (which may have missed something) if we want to allow mutable implementations, then the tests you suggest don't work.

For example, if I do this test:

fn turning_right_does_not_change_position() {
    let robot = Robot::new(0, 0, &Direction::North);
    let robot = robot.turn_right();
    assert_eq!((0, 0), robot.position());
}

and this implementation

pub fn turn_right(&mut self) -> &mut Self {
        self.position = Position::new(1, 1);
        self
}

Then I get the compiler error

tests/robot-simulator.rs:8:17: 8:22 error: cannot borrow immutable local variable `robot` as mutable
tests/robot-simulator.rs:8     let robot = robot.turn_right();
                                           ^~~~~
tests/robot-simulator.rs:7:9: 7:14 help: to make the local variable mutable, use `mut` as shown:
tests/robot-simulator.rs:      let mut robot = Robot::new(0, 0, &Direction::North);

And the compiler is right, if I change the test to

let mut robot = Robot::new(0, 0, &Direction::North);

Then we're cool.

The current test structure does already allow for the first two function signatures. But I don't know of a way to also allow mutability without mutability being declared in the tests.

@IanWhitney

IanWhitney Jul 6, 2016

Contributor

In my testing (which may have missed something) if we want to allow mutable implementations, then the tests you suggest don't work.

For example, if I do this test:

fn turning_right_does_not_change_position() {
    let robot = Robot::new(0, 0, &Direction::North);
    let robot = robot.turn_right();
    assert_eq!((0, 0), robot.position());
}

and this implementation

pub fn turn_right(&mut self) -> &mut Self {
        self.position = Position::new(1, 1);
        self
}

Then I get the compiler error

tests/robot-simulator.rs:8:17: 8:22 error: cannot borrow immutable local variable `robot` as mutable
tests/robot-simulator.rs:8     let robot = robot.turn_right();
                                           ^~~~~
tests/robot-simulator.rs:7:9: 7:14 help: to make the local variable mutable, use `mut` as shown:
tests/robot-simulator.rs:      let mut robot = Robot::new(0, 0, &Direction::North);

And the compiler is right, if I change the test to

let mut robot = Robot::new(0, 0, &Direction::North);

Then we're cool.

The current test structure does already allow for the first two function signatures. But I don't know of a way to also allow mutability without mutability being declared in the tests.

This comment has been minimized.

@Ryman

Ryman Jul 6, 2016

Ah yes, you're totally correct! I forgot that would be required!

I guess that silencing the "unnecessary mut" warnings with an attribute, or even having mut in the tests at all whenever the user chooses to do things with copies could be confusing so it's probably best to leave things as they are then. It's a shame as I feel the mutable implementation can be more idiomatic here.

@Ryman

Ryman Jul 6, 2016

Ah yes, you're totally correct! I forgot that would be required!

I guess that silencing the "unnecessary mut" warnings with an attribute, or even having mut in the tests at all whenever the user chooses to do things with copies could be confusing so it's probably best to leave things as they are then. It's a shame as I feel the mutable implementation can be more idiomatic here.

Show outdated Hide outdated exercises/robot-simulator/example.rs
impl Direction {
pub fn previous_clockwise(&self) -> Self {
match self {

This comment has been minimized.

@Ryman

Ryman Jul 5, 2016

nit: These matches are generally written in the form:

match *self {
    Direction::North => Direction::West,
    ...
}

(Note the lack of reference sigils in the patterns)

@Ryman

Ryman Jul 5, 2016

nit: These matches are generally written in the form:

match *self {
    Direction::North => Direction::West,
    ...
}

(Note the lack of reference sigils in the patterns)

}
pub fn advance(&self) -> Self {
Self::build(self.position.advance(&self.direction), self.direction)

This comment has been minimized.

@Ryman

Ryman Jul 5, 2016

This is personal preference but I find these one liners rather noisy and think separating it out is clearer to see what's happening:

let mut next = self.clone();
next.position = next.position.advance(self.direction);
next

Or to pattern match it into components and rebuild:

let Robot { mut position, direction } = self.clone();
position = position.advance(direction);
Self::build(position, direction)

Or with functional update syntax:

Robot {
    position: self.position.advance(&self.direction),
    // makes it more clear that only the position property is changing
    .. self.clone() 
}
@Ryman

Ryman Jul 5, 2016

This is personal preference but I find these one liners rather noisy and think separating it out is clearer to see what's happening:

let mut next = self.clone();
next.position = next.position.advance(self.direction);
next

Or to pattern match it into components and rebuild:

let Robot { mut position, direction } = self.clone();
position = position.advance(direction);
Self::build(position, direction)

Or with functional update syntax:

Robot {
    position: self.position.advance(&self.direction),
    // makes it more clear that only the position property is changing
    .. self.clone() 
}
pub fn instructions(&self, instructions: &str) -> Self {
instructions.chars().fold(self.clone(),
|robot, instruction| robot.execute(instruction))

This comment has been minimized.

@Ryman

Ryman Jul 5, 2016

nit: formatting of the closure is a bit unexpected, usually I see this formatted as:

instructions.chars().fold(self.clone(), |robot, instruction| {
    robot.execute(instruction)
})
@Ryman

Ryman Jul 5, 2016

nit: formatting of the closure is a bit unexpected, usually I see this formatted as:

instructions.chars().fold(self.clone(), |robot, instruction| {
    robot.execute(instruction)
})

This comment has been minimized.

@IanWhitney

IanWhitney Jul 6, 2016

Contributor

All the formatting is being done automatically by rustfmt. I try not to fight it.

@IanWhitney

IanWhitney Jul 6, 2016

Contributor

All the formatting is being done automatically by rustfmt. I try not to fight it.

This comment has been minimized.

@Ryman

Ryman Jul 6, 2016

Fair enough! :)

@Ryman

Ryman Jul 6, 2016

Fair enough! :)

'R' => self.turn_right(),
'L' => self.turn_left(),
'A' => self.advance(),
_ => self,

This comment has been minimized.

@Ryman

Ryman Jul 5, 2016

Should there be a test exercising 'unknown' commands? (I would expect this to panic, or propagate an error value)

@Ryman

Ryman Jul 5, 2016

Should there be a test exercising 'unknown' commands? (I would expect this to panic, or propagate an error value)

This comment has been minimized.

@IanWhitney

IanWhitney Jul 6, 2016

Contributor

That's a good suggestion for the common test suite

@IanWhitney

IanWhitney Jul 6, 2016

Contributor

That's a good suggestion for the common test suite

This comment has been minimized.

@Ryman

Ryman Jul 6, 2016

The suite mentions the possibility to have an optional test for "an invalid instruction throws an error".

@Ryman

Ryman Jul 6, 2016

The suite mentions the possibility to have an optional test for "an invalid instruction throws an error".

@Ryman

This comment has been minimized.

Show comment
Hide comment
@Ryman

Ryman Jul 5, 2016

Sorry for the delayed response, I left some feedback on things which may affect the users code and just left some nits on the example itself.

Ryman commented Jul 5, 2016

Sorry for the delayed response, I left some feedback on things which may affect the users code and just left some nits on the example itself.

IanWhitney pushed a commit to IanWhitney/xrust that referenced this pull request Jul 6, 2016

@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Jul 6, 2016

Contributor

Unless there's more discussion on this, I'll merge tonight.

Contributor

IanWhitney commented Jul 6, 2016

Unless there's more discussion on this, I'll merge tonight.

Ian Whitney
Implement Robot Simulator
Tests come from
https://github.com/exercism/x-common/blob/183934754b1847612809db410a8aeb640678f188/robot-simulator.json

As discussed in #117 we are
providing a stub implementation so that ignored tests do not fail when a
student hasn't yet implemented the functionality they exercise.

A lot of excellent feedback and help provided by

- jonasbb
- petertseng
- steveklabnik
- Ryman
- Dr-Emann

Full details of the design discussion at

#146

--

Some details that people may care about:

----

Robots are immutable because:

- Immutability is the default in Rust
- No other problem (that I know of) really features Rust's default immutability
- Immutability is not my natural inclination

I figure if I don't expect immutability then other programmers with an
OO-focused background probably don't expect immutability. So a problem
that forces immutability may make me (and them) think about
immutability. Which I think is good.

Immutability brings other benefits, though they aren't exposed by the
tests. It would be very easy to trace your Robot's path, for example.
Re-winding to a specific point on the path would be trivial. All very
nice, but not part of the test suite.

----

The example code has the `build` function because:

There's still some awkwardness around the `new` function, since it takes
x/y and all the internals use Position.

I have to preserve the `new(x, y, direction)` function signature because
of the tests (and because the tests should follow the example of Queen
Attack, which also passes `x` and `y` in as separate parameters.

But that function becomes a pain once I'm working inside the Robot and I
have a Position.

The new `build` function creates a robot using a Position and Direction,
so it can be easily used by all of the Robot's internals. And `new` now
just wraps around `build`.

This is kind of the exact opposite way I'd normally do this. I'd expect
a `build` function to be the public API, do the data manipulation and
then call a private `new` function.

But if I were to do that here then all of the tests would contain:

```
Robot::build(x, y, &direction)
```

Which I think would be non-idiomatic and weird to students.

----
@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Jul 7, 2016

Contributor

And in it goes. Thanks for all your input!

Contributor

IanWhitney commented Jul 7, 2016

And in it goes. Thanks for all your input!

@IanWhitney IanWhitney merged commit 3597821 into exercism:master Jul 7, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@IanWhitney IanWhitney deleted the IanWhitney:implement_robot_simulator branch Jul 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment