Idiom: Prefer Guard Clause over Nested Conditionals

[danielpclark]

Guard Clause

I'm taking my inspiration from Ruby for this one.

Avoid use of nested conditionals for flow of control. Prefer a guard clause when you can assert invalid data. A guard clause is a conditional statement at the top of a function that bails out as soon as it can. - Ruby Style Guide @ No Nested Conditionals

First I'll provide three examples of the wrong way to do it. These are overly nested and trying to follow the paths or guess with else belongs to what can be confusing at a glance. These examples are a simple file config parse

Wrong (A)

fn read_config1() -> Result<Config, Error> {
  let file = File::open("program.cfg");
  if let Ok(f) = file {
    let mut buf_reader = BufReader::new(f);
    let mut contents = String::new();

    if buf_reader.read_to_string(&mut contents).is_ok() {
      let mut data: Vec<u8> = vec![];

      for item in contents.
          split("\n").
          map(|s| s.to_string()).
          filter(|s| !s.is_empty()).
          collect::<Vec<String>>() {

        let num = item.parse::<u8>();

        if let Ok(conf) = num {
          data.push(conf);
        } else {
          return Err(Error::ConfigParseFail);
        }
      }

      Ok( Config { data: data } )
    } else {
      Err(Error::ConfigLoadFail)
    }
  } else {
    Err(Error::ConfigLoadFail)
  }
}

Wrong (B)

fn read_config2() -> Result<Config, Error> {
  let file = File::open("program.cfg");
  match file {
    Ok(f) => {
      let mut buf_reader = BufReader::new(f);
      let mut contents = String::new();

      match buf_reader.read_to_string(&mut contents) {
        Ok(_) => {
          let mut data: Vec<u8> = vec![];

          for item in contents.
              split("\n").
              map(|s| s.to_string()).
              filter(|s| !s.is_empty()).
              collect::<Vec<String>>() {

            let num = item.parse::<u8>();

            match num {
              Ok(conf) => data.push(conf),
              _ => { return Err(Error::ConfigParseFail); },
            }
          }

          Ok( Config { data: data } )
        },
        _ => { Err(Error::ConfigLoadFail) }
      }
    },
    _ => { Err(Error::ConfigLoadFail) }
  }
}

Wrong (C)

fn read_config3() -> Result<Config, Error> {
  let file = File::open("program.cfg");

  if let Ok(f) = file {
    let mut buf_reader = BufReader::new(f);
    let mut contents = String::new();

    if buf_reader.read_to_string(&mut contents).is_ok() {
      let mut data: Vec<u8> = vec![];

      for item in contents.
          split("\n").
          map(|s| s.to_string()).
          filter(|s| !s.is_empty()).
          collect::<Vec<String>>() {

        let num = item.parse::<u8>();

        if let Ok(conf) = num {
          data.push(conf);
        } else {
          return Err(Error::ConfigParseFail);
        }
      }

      return Ok( Config { data: data } );
    }
  }

  Err(Error::ConfigLoadFail)
}

And here is the correct usage of a Guard Clause which allows us to avoid deeply nested logic.

Correct

fn read_config4() -> Result<Config, Error> {
  let file = File::open("program.cfg");

  // Correct use of Guard Clause
  if let Err(_) = file { return Err(Error::ConfigLoadFail); }

  let f = file.unwrap();

  let mut buf_reader = BufReader::new(f);
  let mut contents = String::new();

  // Correct use of Guard Clause
  if let Err(_) = buf_reader.read_to_string(&mut contents) {
    return Err(Error::ConfigLoadFail);
  }

  let mut data: Vec<u8> = vec![];

  for item in contents.
      split("\n").
      map(|s| s.to_string()).
      filter(|s| !s.is_empty()).
      collect::<Vec<String>>() {

    let num = item.parse::<u8>();

    match num {
      Ok(conf) => data.push(conf),
      Err(_) => { return Err(Error::ConfigParseFail); }
    }
  }

  Ok( Config { data: data } )
}

_If you'd like to test the examples above I have a working gist here: danielpclark/guard_clause_rfc.rs … you just need to create the config file with the contents of "1\n2\n3\n" ._

Having your code implemented with guard clauses adds a huge benefit for code clarity and avoids having your conditionals carry your code too far and deep to the right.

Improvements for Guard Clause is currently in Pre-RFC as I would like to avoid using unwrap() after a Guard Clause.

The guard clause doesn't have to be strictly for an Error alternate return type. I believe it can apply for any time when there are two conditional paths and one of those paths is nothing but a return type.

[erickt]

My preference is to use ? when possible and implement the appropriate From<...> for Error, which allows us to avoid .unwrap():

fn read_config4() -> Result<Config, Error> {
  let file = File::open("program.cfg")?;

  let mut buf_reader = BufReader::new(file);
  let mut contents = String::new();

  // Correct use of Guard Clause
  buf_reader.read_to_string(&mut contents)?;

  let mut data: Vec<u8> = vec![];

  for item in contents.
      split("\n").
      map(|s| s.to_string()).
      filter(|s| !s.is_empty()) {
    let num = item.parse::<u8>();
    data.push(num?);
  }

  Ok( Config { data: data } )
}

This makes the code much easier to read. For cases where it's not possible to use ? (like if the Error type is defined outside of the current crate), I prefer this pattern to reduce drift and avoid .unwrap():

fn read_config4() -> Result<Config, Error> {
  let file = File::open("program.cfg");

  // Correct use of Guard Clause
  let f = match file {
    Ok(f) => f,
    Err(_) => { return Err(Error::ConfigLoadFail); }
  };

  // We don't always need guard clauses.
  if buf_reader.read_to_string(&mut contents).is_err() {
    return Err(Error::ConfigLoadFail);
  }

  ...
}

[danielpclark]

I'm not sure handling assignment during the guard condition qualifies as a guard clause. Typically a Guard is at the forefront and indicates the alternate path that is handled before the safe assignment occurs. That's one reason why I didn't include kennytm's great acceptable use of map_err in the Guard Clause examples.

let f = file.map_err(|_| Error::ConfigLoadFail)?;

I think this is beautiful and acceptable for being preferred over nested conditionals, but I don't consider this a Guard Clause anymore. Yes I would openly advise this kind of usage, but I still firmly believe a Guard Clause would be far more beneficial to the language for two reasons:

• first it's easier to read and comprehend which leads me to the second reason
• it makes learning Rust and coming from other languages would be simpler to do and easier to learn.

I believe the term Guard Clause would better be defined as “A pre-assignment condition check with an explicit return. The purpose of which provides first and foremost continued type/condition safe code execution in the same scope. And secondly for code clarity and readability.” These terms are more generic for any language including non-typed languages.

I'm looking forward to Rust enabling Pattern Matching after Guard Clause where leanardo described it as enabling True Flow Typing.

match and map_err are acceptable usages in Rust but I don't believe they fit the definition of a Guard Clause. And where there are many times when these are perfect to use in Rust I don't believe we should get carried away in using them everywhere. Future developers will thank us when we prioritize code clarity over mechanics.

[danielpclark]

// We don't always need guard clauses.

if buf_reader.read_to_string(&mut contents).is_err() {
return Err(Error::ConfigLoadFail);
}

I consider that a Guard Clause.

[danielpclark]

I had written on RFC to add Flow Typing to allow guard clauses to handle one enum path within scope but that got rejected. I go into much more detail about Guard Clauses there: https://github.com/rust-lang/rfcs/pull/2221

[danielpclark]

Without True Flow Typing this practice isn't as convenient. It still stands true that it's a much better pattern to help new developers write simpler and more composable code. So I'll close this issue as it will still be hear for educational purposes but fundamentally needs Rust to make a step towards simplifying the language so we can all simplify our code for better readability.