Best way to refactor this code


#1

Trying to learn more about rust patterns.

The following list has a lot of duplicated code. What’s the best way to refactor it?

Thanks

struct Shape {
    x: i32,
    y: i32
}

struct Circle {
    x: i32,
    y: i32,
    radious: i32
}

struct Square {
    x: i32,
    y: i32,
    width: i32,
    height: i32
}

trait Advance {
    fn advance(&mut self);
}

impl Advance for Shape {
    fn advance(&mut self) {
        self.x += 1;
    }
}

impl Advance for Square {
    fn advance(&mut self) {
        self.x += 1;
    }
}

impl Advance for Circle {
    fn advance(&mut self) {
        self.x += 1;
    }
}

fn main() {
    println!("Hello World!!");
    
    let mut shape = Shape{x: 0, y: 0};
    let mut square = Square{x: 0, y: 0, width: 10, height: 10};
    let mut circle = Circle{x: 0, y: 0, radious: 5};
    
    while shape.x < 10 {
        shape.advance();
        circle.advance();
        square.advance();
        println!("shape {}", shape.x);
        println!("circle {}", circle.x);
        println!("square {}", square.x);
        println!("---------------------");
    }
}

(Playground)


#2

Hi i new in rust but try to suggest this solution (i got error when i try to access advance function but may be there is solution that i don’t know, edit: i fixed the error by using generic T )
if i understand right the problem because there is no way (that i know) to share same functionally of some structs with the same fields unless you wrap functions that receive the needed fields
I will be nice if there is way to map struct fields to trait fields and save all common functionally (related to specific trait ) in that trait

trait Advance {
    fn advance(&mut self);
}

trait TShape {
    fn get_x_mut(&mut self) -> &mut i32;
    fn get_y_mut(&mut self) -> &mut i32;
}

// I using in T to fix the error i got befor
impl <T:TShape> Advance for T {
    fn advance(&mut self) {
        *self.get_x_mut() += 1;
    }
}

struct Shape {
    x: i32,
    y: i32,
}

impl TShape for Shape {
    fn get_x_mut(&mut self) -> &mut i32 {
        &mut self.x
    }
    fn get_y_mut(&mut self) -> &mut i32 {
        &mut self.y
    }
}

struct Circle {
    x: i32,
    y: i32,
    radious: i32,
}

impl TShape for Circle {
    fn get_x_mut(&mut self) -> &mut i32 {
        &mut self.x
    }
    fn get_y_mut(&mut self) -> &mut i32 {
        &mut self.y
    }
}

struct Square {
    x: i32,
    y: i32,
    width: i32,
    height: i32,
}

impl TShape for Square {
    fn get_x_mut(&mut self) -> &mut i32 {
        &mut self.x
    }
    fn get_y_mut(&mut self) -> &mut i32 {
        &mut self.y
    }
}

fn main() {
    println!("Hello World!!");

    let mut shape = Shape { x: 0, y: 0 };
    let mut square = Square {
        x: 0,
        y: 0,
        width: 10,
        height: 10,
    };
    let mut circle = Circle {
        x: 0,
        y: 0,
        radious: 5,
    };

    while shape.x < 10 {
        shape.advance();
        circle.advance();
        square.advance();
        println!("shape {}", shape.x);
        println!("circle {}", circle.x);
        println!("square {}", square.x);
        println!("---------------------");
    }
}

#3

I don’t really know what exactly would you achieve, but I will probably separate Shape, from Coordinates, and keep them alongside. In such approach, you will have something like: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=50b8f834d9efa4ea89db4ed792b63b56. Now you may keep shape and position in some other struct (ShapedObject here), they may be components in ECS, you also may create something which has one of position/shape, but not the other one. You also may still extract your advance method to a trait, and even implement it for ShapedObject redirecting call to position. Many options, complete flexibility.


#4

This approach is nice but force you to split your structs


#5

You could also use an enum here:

struct Position {
    x: i32,
    y: i32,
}

enum ShapeType {
    None,
    Circle { radius: i32 },
    Square { width: i32, height: i32 },
}

struct Shape {
    position: Position,
    shape_type: ShapeType,
}

impl Shape {
    fn new(x: i32, y: i32, shape_type: ShapeType) -> Shape {
        Shape {
            position: Position { x, y },
            shape_type,
        }
    }

    fn advance(&mut self) {
        self.position.x += 1;
    }
}

fn main() {
    let mut shape = Shape::new(0, 0, ShapeType::None);
    let mut square = Shape::new(
        0,
        0,
        ShapeType::Square {
            width: 10,
            height: 10,
        },
    );
    let mut circle = Shape::new(0, 0, ShapeType::Circle { radius: 5 });

    while shape.position.x < 10 {
        shape.advance();
        square.advance();
        circle.advance();

        println!("shape {}", shape.position.x);
        println!("circle {}", circle.position.x);
        println!("square {}", square.position.x);
        println!("---------------------");
    }
}

This is by design, to a degree - Rust encourages you to build things using composition (‘has a’ relationships) rather than through inheritance (‘is a’ relationships). That said, there’s been talk of adding some syntax sugar to make it easier to ‘delegate’ behavior to a field, but that’s still in early discussions.


#6

Yes it is, and it is good thing. I mean obviously, in the case when you have structs from external library and you want to implement some trait for them, it will not work, but I don’t think it is a case (if it is, author definetelly should point this out). If it is all OP code, is good to stick to single responsibility rule from the beggining. Shape is shape, and it has nothing to do with position. It may be some entity which has both, but this is great place for composition.


#7

One workaround that can be used to give access to some fields in a trait are view structs (see also After NLL: Interprocedural conflicts).

I’ve refactored the example to use them, using macros to avoid some repetition: playground link

Might be overkill for this situation, for example I prefer @17cupsofcoffee’s solution here, nevertheless I think this is a useful thing to know.


#8

what about something like that: (not composition and not inheritance, simple sharing some functionality base on trait and common fields)

trait Advance {
    fn advance(&mut self);
}

trait Position {
    where self {
        x: mut i32, 
        y: i32,
    }
}

impl Advance for Position {
    fn advance(&mut self) {
        self.x += 1;
    }
}

struct Shape {
    x: i32,
    radious: i32,
}

impl Position for Circle {
    map self {
        y: |S| $.x + $.radious
    }
}

struct Circle {
    circle_x: i32,
    y: i32,
    radious: i32,
}

impl Position for Circle {
    map self {
        x: $.circle_x
    }
}

struct Square {
    x: i32,
    Square_y: i32,
    width: i32,
    height: i32,
}

impl Position for Square {
    map self {
        y: $.Square_y
    }
}

#9

There was actually a ‘fields in traits’ RFC a little while back that proposed something similar :slight_smile: It got postponed as the relevant people didn’t have the bandwidth to continue working on it, but it’s not outside the realms of possibility that something like this gets added to Rust one day.

See also the Delegation RFC, which is somewhat related (and still ongoing).


#10

I very dislike this approach, because I like separation between data, and behaviour. Types (which includes structs, enums, arrays, tuples, whatever would appear in future) are describing exact data storage/layout, and are very concrete. Traits are describing capability of doing something, but from traits POV it should never matter, if field is present on the type, or if its computer every time someone asks for it, or maybe its shared (or even constant) between all type instantions. I know, there was a similar proposal, but it was postponed and I would never use it for such cases. If you really want to “traitize” position, your previous approach looks more idiomatic for me (i would probably split those methods to fn x(&self) -> u32/fn set_x(&mut self, x: u32), but this is another discussion). However I still strongly recommend split Shape part from Coords part on some level - I prefer solution I proposed, but @17cupsofcoffee solution is also nice for me, and for some cases I would take this approach.


#11

I agree about separation between data, and behavior, but the second suggestion is only syntax sugar to the first suggestion but instead implement multi function get & set, map the struct fields to the fields that needed by the trait.


#12

Dear all, thanks for your answers.

What I’m trying to achieve is create a default trait that can access to the properties of the struct so I can impl to any struct that have ‘x’ property without rewriting any code. Let’s take this as an example. As you can see, the advance have a default behavior, but does not access to the properties of self(since the trait does not know that the trait have ‘x’ property). The missing part of the puzzle is how to tell the trait that self have a ‘x’ property that you can work with.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=234eade8f166eed9c32c10291bb0790d

struct Shape {
    x: i32
}

struct Circle {
    x: i32
}

trait Advance {
    fn advance(&mut self) {
        println!("DEFAULT TRAIT!!");
    }
}

impl Advance for Shape {
    
}

impl Advance for Circle{
    
}

fn main() {
    println!("Hello World!!");
    let mut circle = Circle{x: 100};
    let mut shape = Shape{x: 50};
    
    circle.advance();
    shape.advance();
}

#13

Take a look at custom derive, its seems to be what you want.


#14

Based on your description, I’d consider making a trait with an accessor for x and blanket-impl Advance for all T that implements this new trait.

trait X {
    fn x(&mut self) -> &mut i32;
}

impl<T: X> Advance for T {
    fn advance(&mut self) {
        println!("Oh, x is {:?}", self.x());
    }
}

impl X for Circle {
    fn x(&mut self) -> &mut i32 {
        &mut self.x
    }
}

impl Advance for Shape {}

In this example, Shape will use the default impl and Circle will use the blanket impl for all T: X. If it gets tiresome writing impl X for Foo { ... } a lot, I would use a macro for that.

But this feels a lot like one of those toy examples you find in mediocre computer science books trying to explain what inheritance is. In other words, demonstrating the features of the language, but not a good example of real-world programming. And Rust doesn’t have inheritance, so the example is going to look clumsy compared to the same thing in, say, Java, because the example was probably invented to demonstrate a feature that Java has and Rust lacks.

So I don’t find much value in these kinds of toy examples. I can say “composition over inheritance” until I’m blue in the face, but if there’s no concrete code in front of me I can’t make concrete suggestions for improving it.


#15

In my first suggestion i mean something like that but i got a error, so i fixed it by using T like your example thanks (It will helpful to me)


#16

Hi @trentj, thanks for your example. I’ll try to explain what I’m trying to achieve.

I want to make a game using sfml. I’m trying to create GameObjects that I can add behaviors that do very little and compose them to create more complex behaviors.

The test case, is create a struct that reflects the position, the size and some canvas or view.

I was trying to implements this behaviors using traits. I’m not coming from Java or C++, but from Javascript.

I used to work with PIXIJS, which requires very little code compared with rust. This “trait definition/implementation” it’s really hard for me to understand since I have to re-write code all the time.

What I’m trying to say is: This is NOT toy code. It’s the real implementation. I’m starting. I need to solve this behavior attaching task in a confortable way to keep building in top if it.

I’ll show you the full code, but it’s the same + smfl boilerplate and initialization tasks(that will not run on the playground).

Here it is.

Hopefully this context will help to understand why I try to solve this rewriting as less code as I can.

Thanks again.


#17

I’d like to do something like “Look trait. Trust me, Self have a property called x, don’t complain about it, compile, it will work”.


#18

You right, but in some context you don’t want a pure shape in this case the only fields for the shape are x and y so this is not a shape this is more like position but it call shape so i assume in this context that all the shape included a position in it.


#19

I’d recommend watching this talk from RustConf 2018 on how to structure this kind of thing in Rust - I’m also working on gamedev stuff at the minute and I found it very helpful :slight_smile:


#20

Yes, thanks, I think for what I want to do, I can totally forget about traits. This approach seems cool.

I just wanted to use the game as an excuse to learn rust, but I need to think more in a data oriented thing.