rust-unofficial / patterns

A catalogue of Rust design patterns, anti-patterns and idioms
https://rust-unofficial.github.io/patterns/
Mozilla Public License 2.0
8.02k stars 366 forks source link

Do you consider trait-to-extend as a anti-pattern #310

Closed sudipghimire533 closed 1 year ago

sudipghimire533 commented 2 years ago

Trait-to-extend ( I don't know what else could I name it )

I am presenting my opinion for another anti-pattern. While I have treated this as anti-pattern some of my friend says it might be pattern instead. So depending upon the discussion we might add it to (anti-)pattern section

Creating a new trait to extend external struct functionality

The pattern

Suppose this is the content of struct Time in any external crate:

enum Color {
    Red, Green, Blue, Yellow, Black, White, Gray, Custom(u8, u8, u8)
}

struct Style {
    background: Color,
    foreground: Color,
}

impl Style {
    pub fn new(fg: Color, bg: Color, ...other_properties) -> {
        Style {
           foreground: fg, background: bg,
        }
    }
}

And we are using this crate. Suppose now we start to use this Style struct. Now in this application we need to only create Dark style i.e {fg: White. bg: Black} and Light style i.e { fg: Black, bg: White }. What I previoudly did and some of my colleges still do is something like:

use other_crate::{
    Style,
    Color::{Black, White},
};

pub trait LightAndDarK {
  fn get_light(..other_properties) -> Style;
  fn get_dark(..other_properties) -> Style;   
}

impl LightAndDark for Style {
   fn get_light(..other_properties) -> Style {
        Style::new(Black, White)
    }

    fn get_dark(..other_properties) -> Style {
        Style::new(White, Black)
    }
}

fn main() {
    let light_style = Style::get_light(..);
    let dark_style = Style::get_dark(..);
}

I Consider this as anti-pattern. What I prefer a better idea for such case is to:

use other_crate::{
    Style,
    Color::{Black, White},
};

pub struct DarkStyle;
impl DarkStyle {
    pub fn new(..other_prop) -> Style { Style::new(White, Black) }
}

pub struct LightStyle;
impl DarkStyle {
    pub fn new(..other_prop) -> Style { Style::new(Black, White) }
}

fn new() {
    let light_style = LightStyle::new();
    let dark_style = DarkStyle::new();
}

Here the main idea is: there is not a method we want in external struct, since we can't extend external item create a new local trait with desired method and implement it to external item.

What do you think of this? And also if maintainers are willing to add this (anti-)pattern, I would be glad to take responsibility to write this

neithernut commented 2 years ago

Well, an "extension trait" is the most sane solution to extend foreign structs (e.g. ones from another crate that you have no control over) that I' aware of. So in this case I wouldn't consider this an anti-pattern.

For types in the same crate, it surely is questionable why you'd provide functionality for one specific type via a trait, imo. Of course, if you can, in principle, implement the trait to other types in a sensible way it's a whole different story. And this may include cases where the functionality is, in principle, still pinned on some struct.

For example, I'm currently working on a little thingy where I want to support accessing some struct Buffer both from multiple threads and a single thread through some struct Extender without needing to carry around the weight of a mutex in the latter case. So I ended up implementing a trait Accumulator for my actual target (e.g. struct Buffer), but also for Arc<Mutex<Buffer>>. So you could consider Accumulator an "extension trait", but there's still merit in having it this way rather than having implementing the functionality directly on Buffer.

sudipghimire533 commented 2 years ago

If we think it is preferred solution, will it make sense to add to "pattern" section?