4

Problem Statement

I have a set of structs, A, B, C, and D, which all implement a trait Runnable.

trait Runnable {
    fn run(&mut self);
}
impl Runnable for A {...}
impl Runnable for B {...}
impl Runnable for C {...}
impl Runnable for D {...}

I also have a struct Config which serves as the specification for constructing A, B, C, and D instances.

struct Config {
    filename: String,
    other_stuff: u8,
}

impl From<Config> for A {...}
impl From<Config> for B {...}
impl From<Config> for C {...}
impl From<Config> for D {...}

In my program, I would like to parse a Config instance and construct either an A, B, C, or D depending on the value of the filename field, and then call Runnable::run on it. The struct should be selected by sequentially checking each struct against a filename string and selecting the first one that "matches" that string.

Naïve Implementation

Here is a naïve implementation.

trait CheckFilename {
    fn check_filename(filename: &str) -> bool;
}
impl CheckFilename for A {...}
impl CheckFilename for B {...}
impl CheckFilename for C {...}
impl CheckFilename for D {...}


fn main() {
    let cfg: Config = get_config(); // Some abstract way of evaluating a Config at runtime.

    let mut job: Box<dyn Runnable> = if A::check_filename(&cfg.filename) {
        println!("Found matching filename for A");
        Box::new(A::from(cfg))
    } else if B::check_filename(&cfg.filename) {
        println!("Found matching filename for B");
        Box::new(B::from(cfg))
    } else if C::check_filename(&cfg.filename) {
        println!("Found matching filename for C");
        Box::new(C::from(cfg))
    } else if D::check_filename(&cfg.filename) {
        println!("Found matching filename for D");
        Box::new(D::from(cfg))
    } else {
        panic!("did not find matching pattern for filename {}", cfg.filename);
    };

    job.run();
}

This works but has some code smells:

  • Giant if else if else if else if else... statement is smelly IMO
  • Lots of repetition: The code used to check the filename, print which struct type was selected, and construct the instance from the config are the same for each branch and only differ in which struct type they're dealing with. Is there a way to abstract this repetition away?
  • Very error prone: its very easy to accidentally screw up the mapping between filename string and struct by failing to synchronize the struct with the predicate; for example writing something like:
    if D::check_filename(&cfg.filename) {
        println!("Found matching filename for D");
        Box::new(B::from(cfg)) // Developer error: constructs a B instead of a D.
    }
    
    and the compiler would not catch this.
  • Adding new structs (e.g. E, F, G, etc.) to the program is not very ergonomic. It requires adding a new branch for each to the main if else statement. It'd be much nicer to simply add the struct to some sort of "master list" of srtuct types.

Is there a more elegant or idiomatic way of doing this that addresses these smells?

2 Answers 2

8

Since the conversion consumes Config, the challenge in unifying the logic for all types is that you need to conditionally move the config value in order to convert it. The standard library has multiple cases of fallible consuming functions and the pattern they use is to return Result, handing back the maybe-consumed value in the Err case. For example, Arc::try_unwrap extracts the inner value of an Arc, but if this fails it gives the Arc back in the Err variant.

We can do the same here, creating a single function that produces one of the appropriate structs if the filename matches, but giving back the config on an error:

fn try_convert_config_to<T>(config: Config) -> Result<Box<dyn Runnable>, Config>
where
    T: Runnable + CheckFilename + 'static,
    Config: Into<T>,
{
    if T::check_filename(&config.filename) {
        Ok(Box::new(config.into()))
    } else {
        Err(config)
    }
}

Then you can write another function with a static slice of specific instantiations of this function, and it can try each in order until one succeeds. Since we move the config into each loader function, we have to put it back in the Err case so the next loop iteration can move it again.

fn try_convert_config(mut config: Config) -> Option<Box<dyn Runnable>> {
    static CONFIG_LOADERS: &[fn(Config) -> Result<Box<dyn Runnable>, Config>] = &[
        try_convert_config_to::<A>,
        try_convert_config_to::<B>,
        try_convert_config_to::<C>,
        try_convert_config_to::<D>,
    ];

    for loader in CONFIG_LOADERS {
        match loader(config) {
            Ok(c) => return Some(c),
            Err(c) => config = c,
        };
    }

    None
}

This solves all of your concerns:

  • There is no longer a giant if-else chain, just a loop.
  • The code duplication is gone because try_convert_config_to implements the logic for all types one time.
  • It's impossible to accidentally invoke the two parts of the process (check_filename and into) on different types as long as you use try_convert_config_to.
  • To add a new type you just need to add a new element to the CONFIG_LOADERS slice.

(Playground)

Sign up to request clarification or add additional context in comments.

3 Comments

Excellent solution. Can you explain why T needs the 'static trait bound in try_convert_config_to?
@RBF06 Because dyn Foo implicitly has a 'static bound unless you specify one, so Box<dyn Runnable> is equivalent to Box<dyn Runnable + 'static>. Therefore, for a Box<T> to be cast to a Box<dyn Runnable>, T must be 'static.
To clarify, "unless you specify one" means "unless you specify a different lifetime." E.g. you can have Box<dyn Runnable + 'a> for some lifetime 'a.
1

Answering my own question to clean up a few minor loose ends left by the accepted answer, and also to provide some additional explanation and references. Shout out to @cdhowie for providing the accepted answer and the knowledge I needed to solve my problem.


Overview

By refactoring a few things, it is possible to solve all the issues with the naïve implementation. The trick is to create a simple fallible conversion function that converts a Config value into a Box<dyn Runnable> if the filename matches, but gives the Config value back in the Err variant if the filename does not match so that the next specialization can try.

Define the requirements

First, let us compile a list of the minimum requirements each type must satisfy in order to support our application:

  1. must be Runnable since we intend to run() it once instantiated.
  2. must be From<Config> because we intend to create one from a Config
  3. must have a name associated with it so we can print the log message identifying which type was selected.
  4. must be able to determine if it "matches" a filename string

With these requirements defined, we define a Trait to enforce them:

trait Job: Runnable + From<Config> {
    const NAME: &'static str;
    fn check_filename(filename: &str) -> bool;
}

Requirement 1 and 2 are encoded using supertraits and requirement 3 is encoded using an associated constant

Note: in the problem statement, this trait was named CheckFilename but it is renamed here to Job because that seems more appropriate considering the additional requirements.

Implementing Job for each of our structs is very straightforward:

impl Job for A {
    // This str will be the used as the name of the struct when printing the log message
    const NAME: &'static str = "A";
    fn check_filename(filename: &str) -> bool {
        // logic determining if a `str` matches a Job type goes here
        filename.starts_with('A')
    }
}
impl Job for B {...}
impl Job for C {...}
impl Job for D {...}

Create a generic converter function

Next, we create a simple fallible function that tries to convert a Config value to a Box<dyn Runnable>, but gives the Config value back (with ownership) if it doesn't match. This function is generic over all Job types.

fn try_convert_config_to<T: Job + 'static>(cfg: Config) -> Result<Box<dyn Runnable>, Config> {
    if T::check_filename(&cfg.filename) {
        println!("Found matching job for filename `{}`: {}", cfg.filename, T::NAME);
        Ok(Box::<T>::new(cfg.into()))
    } else {
        Err(cfg)
    }
}

Chain it all together

Finally, a (boxed) Runnable trait object can be initialized by calling try_convert_config_to::<A> and sequentially passing error values through a chain of Result::or_else with the remaining specializations of Job.

fn main() {
    let cfg: Config = get_config();
    let mut job: Box<dyn Runnable> = try_convert_config_to::<A>(cfg)
        .or_else(try_convert_config_to::<B>)
        .or_else(try_convert_config_to::<C>)
        .or_else(try_convert_config_to::<D>)
        .map_err(|cfg| panic!("Couldn't find matching type for filename `{}`", cfg.filename))
        .unwrap();

    job.run();
}

(Playground)

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.