Traits Workshop

code review
1

I was busy with student-level exercises while I'm self-educating on Rust. This one particular left me wondering if I'm taking correct/optimal path with it. I'd really appreciate all reviews and comments on everything including tips not related to the exercise itself (both here and in the repo itself).

The exercise is focused on traits, starts with replacing enum with trait and then goes into exploring trait object concept.

2

For some initial feedback on minor points, consider using Rust’s linter, clippy

warning: this `impl` can be derived
  --> src/directions/coordinate.rs:20:1
   |
20 | / impl Default for Coordinate {
21 | |     fn default() -> Self {
22 | |         Coordinate { x: 0, y: 0 }
23 | |     }
24 | | }
   | |_^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#derivable_impls
   = note: `#[warn(clippy::derivable_impls)]` on by default
   = help: remove the manual implementation...
help: ...and instead derive it
   |
8  + #[derive(Default)]
9  | pub struct Coordinate {
   |

warning: unneeded `return` statement
  --> src/lib.rs:65:13
   |
65 | /             return if o.is_gravity_source() {
66 | |                 Some((o.get_coordinate().clone(), o.weight().expect(BREAKING_VALUE_KIND)))
67 | |             } else {
68 | |                 None
69 | |             };
   | |_____________^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_return
   = note: `#[warn(clippy::needless_return)]` on by default
   = help: remove `return`

warning: using `clone` on type `Coordinate` which implements the `Copy` trait
  --> src/lib.rs:66:23
   |
66 |                 Some((o.get_coordinate().clone(), o.weight().expect(BREAKING_VALUE_KIND)))
   |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try removing the `clone` call: `o.get_coordinate()`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
   = note: `#[warn(clippy::clone_on_copy)]` on by default

warning: you seem to be trying to use `&Box<T>`. Consider using just `&T`
   --> src/lib.rs:135:26
    |
135 |     let get_circle = |o: &Box<dyn Object>| -> Circle {
    |                          ^^^^^^^^^^^^^^^^ help: try: `&dyn Object`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#borrowed_box
    = note: `#[warn(clippy::borrowed_box)]` on by default

edit: looking at the commits in some branches, it looks like you were already aware of cargo clippy’s existence. The return statement in the closure is present in all branches though, AFAICT, and it hurts a bit to look at.

1 Like
3

Yep, I even returned it back after removing. I tried to avoid unnecessary changes to the initial code as the exercise doesn't tell to refactor the thing. So I treated it as a "code styling that company has"; who am I to dictate them standards. I guess this happens in life too. :thinking:

4

Now I see that branch naming is misleading. =( Is it a fine move to rename published branched?

5

Style:

  • Typically, undecorated words in APIs return references or just data, so a user would expect Object::coordinate to return a Coordinate or an &Coordinate, and for a different method (e.g. coordinate_mut to return a &mut Coordinate).
  • Your code expects gravity-receivers to always have a velocity component and gravity-sources to always have a weight component. Instead of littering apply_physics with expect calls, I would just use the optionality directly:
#[derive(Clone, Copy, Debug)]
pub struct ReceiverData {
    pub velocity: Direction,
    // fields could be hidden behind some API to make changes non-breaking
}

#[derive(Clone, Copy, Debug)]
pub struct SourceData {
    pub weight: i32,
    // fields could be hidden behind some API to make changes non-breaking
}

pub trait Object {
    fn coordinate(&self) -> Coordinate;
    fn coordinate_mut(&mut self) -> &mut Coordinate;
    fn receiver_data_mut(&self) -> Option<&mut ReceiverData>;
    fn source_data_mut(&self) -> Option<&mut ReceiverData>;
}

However, this has the drawback of enforcing that each Object might have a ReceiverData or a SourceData as one of its fields. Setters and getters are an alternate option.

1 Like
6

Looks like much of my confusion whether 'Am I doing it right' comes from the following phrase, which I noticed yesterday, and which I decided to diverge from when doing the workshop.

... refactor the code so that rather than taking a vec of enums, it takes a vec of Planets, and a vec of Asteroids. ...

Thanks a lot for the review! I addressed the naming suggestion; expect issue unfolded quite unexpectedly (pun not much intended). After taking it in straightforward manner I went to the task7 and removed todo!() which covered the very essence of the task under the surface (which is just ability to input trait object into the fn); at that point I felt like it looks like final answer to the exercise and merged it into master to make everything a bit less scattered.

Long story short: it seems to me that current master fixes both issues you pointed me to. Please, reply if I misunderstood anything, and it better be improved (further)!

Also, if you have any other review suggestion(s) I would be happy to do them as well.
Everyone very welcome indeed!

7

That looks great to me!

1 Like