3
\$\begingroup\$

I'm working through Advent of Code 2016 in Rust. The prizes go to the swift, but I'm aiming for well-written code. This is Day 1.

To summarize the problem statement, given a instruction string of turns and distances (e.g. "R5, L5, R5, R3"), return the Manhattan distance from the origin (in this case 12).

I am using Rust version 1.15.0-nightly (71c06a56a 2016-12-18).

lib.rs

#![feature(try_from)]
#![feature(more_struct_aliases)]

mod day_01;

pub fn day_01() {
    let day_01_answer =
        day_01::blocks_away("L4, L1, R4, R1, R1, L3, R5, L5, L2, L3, R2, R1, L4, R5, R4, L2, R1, \
                             R3, L5, R1, L3, L2, R5, L4, L5, R1, R2, L1, R5, L3, R2, R2, L1, R5, \
                             R2, L1, L1, R2, L1, R1, L2, L2, R4, R3, R2, L3, L188, L3, R2, R54, \
                             R1, R1, L2, L4, L3, L2, R3, L1, L1, R3, R5, L1, R5, L1, L1, R2, R4, \
                             R4, L5, L4, L1, R2, R4, R5, L2, L3, R5, L5, R1, R5, L2, R4, L2, L1, \
                             R4, R3, R4, L4, R3, L4, R78, R2, L3, R188, R2, R3, L2, R2, R3, R1, \
                             R5, R1, L1, L1, R4, R2, R1, R5, L1, R4, L4, R2, R5, L2, L5, R4, L3, \
                             L2, R1, R1, L5, L4, R1, L5, L1, L5, L1, L4, L3, L5, R4, R5, R2, L5, \
                             R5, R5, R4, R2, L1, L2, R3, R5, R5, R5, L2, L1, R4, R3, R1, L4, L2, \
                             L3, R2, L3, L5, L2, L2, L1, L2, R5, L2, L2, L3, L1, R1, L4, R2, L4, \
                             R3, R5, R3, R4, R1, R5, L3, L5, L5, L3, L2, L1, R3, L4, R3, R2, L1, \
                             R3, R1, L2, R4, L3, L3, L3, L1, L2");
    assert_eq!(day_01_answer, 279);
}

day_01.rs

use std::convert::TryFrom;

pub fn blocks_away(instructions: &str) -> i16 {
    let mut x = 0;
    let mut y = 0;
    let mut direction = Direction::North;
    for instruction in Instruction::try_many_from(instructions).unwrap() {
        direction.turn(instruction.turn);
        match direction {
            Direction::North => y += instruction.blocks,
            Direction::East => x += instruction.blocks,
            Direction::South => y -= instruction.blocks,
            Direction::West => x -= instruction.blocks,
        }
    }
    x.abs() + y.abs()
}

#[derive(Debug, PartialEq, Eq)]
enum Direction {
    North,
    East,
    South,
    West,
}

impl Direction {
    fn turn(&mut self, turn: Turn) {
        *self = match *self {
            Direction::North => {
                match turn {
                    Turn::Left => Direction::West,
                    Turn::Right => Direction::East,
                }
            }
            Direction::East => {
                match turn {
                    Turn::Left => Direction::North,
                    Turn::Right => Direction::South,
                }
            }
            Direction::South => {
                match turn {
                    Turn::Left => Direction::East,
                    Turn::Right => Direction::West,
                }
            }
            Direction::West => {
                match turn {
                    Turn::Left => Direction::South,
                    Turn::Right => Direction::North,
                }
            }
        }
    }
}

#[derive(Debug, PartialEq, Eq)]
enum Turn {
    Left,
    Right,
}

#[derive(Debug, PartialEq, Eq)]
struct Instruction {
    turn: Turn,
    blocks: i16,
}

impl<'a> Instruction {
    fn try_many_from(s: &'a str) -> Result<Vec<Self>, &'a str> {
        s.split(", ").map(Self::try_from).collect()
    }
}

impl<'a> TryFrom<&'a str> for Instruction {
    type Err = &'a str;

    fn try_from(s: &'a str) -> Result<Self, &'a str> {
        let mut chars = s.chars();
        let turn = match chars.next() {
            Some(turn_char) => {
                match turn_char {
                    'L' => Turn::Left,
                    'R' => Turn::Right,
                    _ => return Err("Turn character invalid"),
                }
            }
            None => return Err("Instruction string is empty"),
        };
        let blocks = try!(chars.as_str().parse::<i16>().map_err(|_| "Could not parse blocks"));
        Ok(Instruction {
            turn: turn,
            blocks: blocks,
        })
    }
}

#[cfg(test)]
mod test {
    use super::*;

    #[test]
    fn test_parse_instruction_success() {
        let instruction = Instruction::try_from("L1").unwrap();
        assert_eq!(instruction,
                   Instruction {
                       turn: Turn::Left,
                       blocks: 1,
                   });
        let instruction = Instruction::try_from("R2").unwrap();
        assert_eq!(instruction,
                   Instruction {
                       turn: Turn::Right,
                       blocks: 2,
                   });
    }

    #[test]
    fn test_parse_instruction_invalid_turn_character() {
        let instruction = Instruction::try_from("S1");
        assert_eq!(instruction, Err("Turn character invalid"));
    }

    #[test]
    fn test_parse_instruction_empty_string() {
        let instruction = Instruction::try_from("");
        assert_eq!(instruction, Err("Instruction string is empty"));
    }

    #[test]
    fn test_parse_instruction_missing_blocks_digit() {
        let instruction = Instruction::try_from("L");
        assert_eq!(instruction, Err("Could not parse blocks"));
    }

    #[test]
    fn test_parse_instruction_invalid_blocks_digit() {
        let instruction = Instruction::try_from("LL");
        assert_eq!(instruction, Err("Could not parse blocks"));
    }

    #[test]
    fn test_parse_instructions() {
        let instructions = Instruction::try_many_from("L1, R2").unwrap();
        assert_eq!(instructions,
                   vec![Instruction {
                            turn: Turn::Left,
                            blocks: 1,
                        },
                        Instruction {
                            turn: Turn::Right,
                            blocks: 2,
                        }]);
    }

    // Assert that parsing failure returns the first error
    #[test]
    fn test_parse_instructions_with_error() {
        let instructions = Instruction::try_many_from("L1, , R2, S2");
        assert_eq!(instructions, Err("Instruction string is empty"));
    }

    #[test]
    fn test_blocks_away() {
        let blocks_away = blocks_away("L4, L1, L1");
        assert_eq!(blocks_away, 4);
    }
}

I'm particularly concerned with the implementation of day_01::Direction::turn. It's quite explicit and symmetric. Is there a more elegant way of implementing it – perhaps with a circular buffer of [North, East, South, West]?

I would have used #![feature(field_init_shorthand)] to replace

Instruction {
    turn: turn,
    blocks: blocks,
}

with Instruction { turn, blocks }, but the current version of rustfmt doesn't support that feature yet.

\$\endgroup\$
3
\$\begingroup\$

day_01.rs

  1. I Would return an error from blocks_away as it's based on input to the function, thus outside of the functions control.
  2. The error and result types are repeated, make type aliases to DRY that up.
  3. Make enums Copy and Clone.
  4. Collapse turn into a single match on a tuple.
  5. Import the enum variants in turn to decrease clutter.
  6. I might just return the new direction instead of modifying self. This is a bit more flexible.
  7. Why is blocks a signed integer?
  8. Collecting to a Vec is unneeded allocation. I'd inline the map or use new feature: conservative_impl_trait!
  9. Should not have <'a> on impl block for Instruction! Instruction has no generic lifetime parameters and it's not a shorthand syntax.
  10. TryFrom<str> already exists: FromStr.
  11. Your error type is a 'static string, not tied to the input string lifetime.
  12. Use Self::Err to avoid repeating in the parsing code return (or the local Result)
  13. Collapse the nested matches in string parsing.
  14. The turbofish when parsing for blocks is unneeded, the type can be inferred.
  15. I prefer avoiding unwrap whenever possible. You can assert_eq! against Ok(...) instead of unwrapping.
use std::str::FromStr;

type Error = &'static str;
type Result<T> = ::std::result::Result<T, Error>;

pub fn blocks_away(instructions: &str) -> Result<i16> {
    let mut x = 0;
    let mut y = 0;
    let mut direction = Direction::North;
    for instruction in Instruction::try_many_from(instructions) {
        let instruction = instruction?;
        direction.turn(instruction.turn);
        match direction {
            Direction::North => y += instruction.blocks,
            Direction::East  => x += instruction.blocks,
            Direction::South => y -= instruction.blocks,
            Direction::West  => x -= instruction.blocks,
        }
    }
    Ok(x.abs() + y.abs())
}

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
enum Direction {
    North,
    East,
    South,
    West,
}

impl Direction {
    fn turn(&mut self, turn: Turn) {
        use self::Direction::*;
        use self::Turn::*;

        *self = match (*self, turn) {
            (North, Left)  => West,
            (North, Right) => East,
            (East,  Left)  => North,
            (East,  Right) => South,
            (South, Left)  => East,
            (South, Right) => West,
            (West,  Left)  => South,
            (West,  Right) => North,
        }
    }
}

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
enum Turn {
    Left,
    Right,
}

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
struct Instruction {
    turn: Turn,
    blocks: i16,
}

impl Instruction {
    fn try_many_from<'a>(s: &'a str) -> impl Iterator<Item = Result<Self>> + 'a {
        s.split(", ").map(str::parse)
    }
}

impl FromStr for Instruction {
    type Err = Error;

    fn from_str(s: &str) -> Result<Self> {
        let mut chars = s.chars();
        let turn = match chars.next() {
            Some('L') => Turn::Left,
            Some('R') => Turn::Right,
            Some(_)   => return Err("Turn character invalid"),
            None      => return Err("Instruction string is empty"),
        };
        let blocks = try!(chars.as_str().parse().map_err(|_| "Could not parse blocks"));
        Ok(Instruction {
            turn: turn,
            blocks: blocks,
        })
    }
}

#[cfg(test)]
mod test {
    use super::*;

    #[test]
    fn test_parse_instruction_success() {
        let instruction = "L1".parse::<Instruction>();
        assert_eq!(instruction,
                   Ok(Instruction {
                       turn: Turn::Left,
                       blocks: 1,
                   }));
        let instruction = "R2".parse::<Instruction>();
        assert_eq!(instruction,
                   Ok(Instruction {
                       turn: Turn::Right,
                       blocks: 2,
                   }));
    }

    #[test]
    fn test_parse_instruction_invalid_turn_character() {
        let instruction = "S1".parse::<Instruction>();
        assert_eq!(instruction, Err("Turn character invalid"));
    }

    #[test]
    fn test_parse_instruction_empty_string() {
        let instruction = "".parse::<Instruction>();
        assert_eq!(instruction, Err("Instruction string is empty"));
    }

    #[test]
    fn test_parse_instruction_missing_blocks_digit() {
        let instruction = "L".parse::<Instruction>();
        assert_eq!(instruction, Err("Could not parse blocks"));
    }

    #[test]
    fn test_parse_instruction_invalid_blocks_digit() {
        let instruction = "LL".parse::<Instruction>();
        assert_eq!(instruction, Err("Could not parse blocks"));
    }

    #[test]
    fn test_parse_instructions() {
        let instructions = Instruction::try_many_from("L1, R2").collect::<Result<Vec<_>>>();
        assert_eq!(instructions,
                   Ok(vec![Instruction {
                            turn: Turn::Left,
                            blocks: 1,
                        },
                        Instruction {
                            turn: Turn::Right,
                            blocks: 2,
                        }]));
    }

    // Assert that parsing failure returns the first error
    #[test]
    fn test_parse_instructions_with_error() {
        let instructions = Instruction::try_many_from("L1, , R2, S2").collect::<Result<Vec<_>>>();
        assert_eq!(instructions, Err("Instruction string is empty"));
    }

    #[test]
    fn test_blocks_away() {
        let blocks_away = blocks_away("L4, L1, L1");
        assert_eq!(blocks_away, Ok(4));
    }
}

lib.rs

  1. The more_struct_aliases feature doesn't appear to be used?
  2. Use include_str! to move giant blob of input out of code and into a file.
#![feature(conservative_impl_trait)]

mod day_01;

pub fn day_01() {
    let day_01_answer =
        day_01::blocks_away("L4, L1, R4, R1, R1, L3, R5, L5, L2, L3, R2, R1, L4, R5, R4, L2, R1, \
                             R3, L5, R1, L3, L2, R5, L4, L5, R1, R2, L1, R5, L3, R2, R2, L1, R5, \
                             R2, L1, L1, R2, L1, R1, L2, L2, R4, R3, R2, L3, L188, L3, R2, R54, \
                             R1, R1, L2, L4, L3, L2, R3, L1, L1, R3, R5, L1, R5, L1, L1, R2, R4, \
                             R4, L5, L4, L1, R2, R4, R5, L2, L3, R5, L5, R1, R5, L2, R4, L2, L1, \
                             R4, R3, R4, L4, R3, L4, R78, R2, L3, R188, R2, R3, L2, R2, R3, R1, \
                             R5, R1, L1, L1, R4, R2, R1, R5, L1, R4, L4, R2, R5, L2, L5, R4, L3, \
                             L2, R1, R1, L5, L4, R1, L5, L1, L5, L1, L4, L3, L5, R4, R5, R2, L5, \
                             R5, R5, R4, R2, L1, L2, R3, R5, R5, R5, L2, L1, R4, R3, R1, L4, L2, \
                             L3, R2, L3, L5, L2, L2, L1, L2, R5, L2, L2, L3, L1, R1, L4, R2, L4, \
                             R3, R5, R3, R4, R1, R5, L3, L5, L5, L3, L2, L1, R3, L4, R3, R2, L1, \
                             R3, R1, L2, R4, L3, L3, L3, L1, L2");
    assert_eq!(day_01_answer, Ok(279));
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you for the excellent code reviews @Shepmaster! The up-to-date code is available on GitHub. I've used your code and performed some of your suggested updates. I originally made blocks a signed integer to avoid casting in blocks_away. I use casting now – a more appropriate choice, I think. \$\endgroup\$ – Matthew Piziak Dec 29 '16 at 16:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.