Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

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

Closed
sudipghimire533 opened this issue Jun 19, 2022 · 2 comments
Closed

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

sudipghimire533 opened this issue Jun 19, 2022 · 2 comments

Comments

@sudipghimire533
Copy link

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
Copy link
Contributor

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
Copy link
Author

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

@rust-unofficial rust-unofficial locked and limited conversation to collaborators Apr 6, 2023
@simonsan simonsan converted this issue into discussion #357 Apr 6, 2023

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants