Trying to refactor a mutable value into an immutable one

I'm making a program that detects what shell it's running in and behaves differently for each. Right now I have a mutable variable shell that stores the current shell based on the environment variable or the user can explicitly override the detection. Since the value of shell won't change once the detection code is done, I wanted to try to refactor it to make it immutable.

This is my current code:

enum Shell {
    sh,
    csh,
    fish,
}

let args: Vec<String> = env::args().collect();

// Default case
let mut shell = Shell::sh;

// Get shell from environment variables
if let Ok(shell_var) = env::var("SHELL") {
    if shell_var.ends_with("fish") {
        shell = Shell::fish;
    } else if shell_var.ends_with("csh") {
        shell = Shell::csh;
    }
}

// User override
if args.len() == 2 {
    shell = match args[1].as_str() {
        "-f" => Shell::fish,
        "-c" => Shell::csh,
        "-s" | _ => Shell::sh,
    };
}

I could do something like this:

let shell = if let Ok(shell_var) = env::var("SHELL") {
    if shell_var.ends_with("fish") {
        Shell::fish
    } else if shell_var.ends_with("csh") {
        Shell::csh
    } else {
        Shell::sh
    }
} else if args.len() == 2 {
    match args[1].as_str() {
        "-f" => Shell::fish,
        "-c" => Shell::csh,
        "-s" | _ => Shell::sh,
    }
} else {
    Shell::sh
};

but then I have 3 separate default cases to assign Shell::sh.

Is there a better way to consolidate this check, or do these conditions just not mesh well together?

In order to make peace with the fact that “the value of shell won't change once the detection code is done”, you could use shadowing and a move to – effectively – turn the mutable shell variable into an immutable one, i.e. add the line

let shell = shell;

at the end of the detection code.

1 Like

I actually find your second solution clearer, I'm not sure what's wrong with it. It clearly expresses what should happen in each case, and I don't have to carry around state and control flow in my head. Just go with the second snippet.

I think your solutions might differ in what they do if both an environment variable and the user override is present.

You can probably do something equivalent to your mutable code, with just two uses of Shell::Sh as well, like this:

let shell = if args.len() == 2 {
    match args[1].as_str() {
        "-f" => Shell::Fish,
        "-c" => Shell::Csh,
        "-s" | _ => Shell::Sh,
    }
} else {
    match env::var("SHELL") {
        Ok(shell_var) if shell_var.ends_with("fish") => Shell::Fish,
        Ok(shell_var) if shell_var.ends_with("csh") => Shell::Csh,
        _ => Shell::Sh,
    }
};

I agree with @H2CO3 that something like this is more readable than the imperative solution.

I changed your enum to use CamelCase variants.


Edit: Hmm… This version, as well as your original code, will choose sh regardless of which arguments were passed as long as it’s a single argument…

2 Likes

I just thought it was kind of ugly and didn't like that I was repeating myself so much.

I've omitted some other error handling code because I didn't want other people to get bogged down in details. Also I'm re-implementing another program and it had some weird quirks in its argument handling, so I had to faithfully copy them.

In any case, I like your solution very much and it behaves as expected when I run it. Thank you!

As a joke, you can also pull a Kevlin Henney on this one – IIRC he was the one who was dissatisfied with how most solutions to FizzBuzz had redundantly repetitive implementations, so he invoked the power of monads in order to compose alternative fallible expressions. The equivalent of that would be something like this:

let shell = env::var("SHELL").ok().and_then(|shell_var| {
    if shell_var.ends_with("fish") {
        Some(Shell::Fish)
    } else if shell_var.ends_with("csh") {
        Some(Shell::Csh)
    } else {
        None
    }
}).or_else(|| args.get(1).and_then(|arg| match arg.as_str() {
    "-f" => Some(Shell::Fish),
    "-c" => Some(Shell::Csh),
    _ => None,
})).unwrap_or(
    Shell::Sh
);
1 Like