6
\$\begingroup\$

I started picking up Rust on my free time, and I'm curious about what style errors have I made and where should I improve as a programmer.

The problem is from HackerRank and very simple: there is a fixed-format input of time in 12 hour format (07:05:45PM), and the objective is to convert it to 24 hour format (19:05:45).

My own first impressions:

  • The problem is very simple, but the code seems too lengthy?
  • I'm nesting match constructs, and it doesn't seem concise.

use std::io::{stdin, BufRead, Error, ErrorKind, Result};

struct Time {
    hour: u8,
    minute: u8, 
    second: u8
}

impl Time {
    pub fn from_12h(time : String) -> Result<Time> {
        assert!(time.chars().count() == 10);
        let hour : String = time.chars().take(2).collect();  
        let minute : String = time.chars().skip(3).take(2).collect();
        let second : String = time.chars().skip(6).take(2).collect();
        let period : String = time.chars().skip(8).take(2).collect();

        match Time::hour_from_12h_to_24h(hour.parse::<u8>().unwrap(), period) {
            Ok(hour_24h) =>
                Ok(Time {
                    hour: hour_24h,
                    minute: minute.parse::<u8>().unwrap(),
                    second: second.parse::<u8>().unwrap()
                }),
            Err(e) => Err(e),
        }
    }

    fn hour_from_12h_to_24h(hour : u8, period : String) -> Result<u8> {
        match period.as_ref() {
            "AM" => Ok(hour % 12),
            "PM" => Ok(hour % 12 + 12),
            _ => Err(Error::new(ErrorKind::InvalidData, format!("invalid period {}", period))),
        }
    }

    pub fn to_24h(&self) -> String {
        format!("{:02}:{:02}:{:02}", self.hour, self.minute, self.second)
    }
}

fn main() {
    match read_12h_time() {
        Ok(time_12h) =>
            match Time::from_12h(time_12h) {
                Ok(time) => println!("{}", time.to_24h()),
                Err(err) => println!("error: {}", err)
            },
        Err(err) => println!("input error: {}", err),
    }
}

fn read_12h_time() -> Result<String> {
    let mut buffer = String::new();
    match read_line(&mut buffer) {
        Ok(length) =>
            match length {
                10 => Ok(buffer),
                _ => Err(Error::new(ErrorKind::InvalidInput, format!("expected 10 characters, got {}", length)))
            },
        Err(e) => Err(e),
    }
}

fn read_line(buffer : &mut String) -> Result<usize> {
    let stdin = stdin();
    let mut handle = stdin.lock();
    handle.read_line(buffer)
}
\$\endgroup\$
4
\$\begingroup\$
  1. Your code doesn't really work when used by humans. The first thing I did was run the code and type in a time, which failed. When a human enters data through the terminal, they will hit Enter, which is returned from read_line:

    $ echo '07:05:45PM' | cargo run
    input error: expected 10 characters, got 11
    $ echo -n '07:05:45PM' | cargo run
    19:05:45
    
  2. Style issues - learn to love rustfmt:

    1. There's no space before : when specifiying a type on a variable.
    2. Use trailing commas.
    3. Match arms longer than one line should use curly braces {}
  3. Accept a &str instead of String; the function doesn't need to take ownership of the string.

  4. There's no need for the turbofish (::<>) on the parse calls; type inference handles that just fine.
  5. Become very familiar with all the methods on Option and Result and Iterator. These are the bread and butter of idiomatic, concise, and performant Rust code. Specifically, if you match on a Result and have a match arm for Ok and Err, it's likely you want to use Result::map.
  6. Avoid iterating through the characters from the beginning multiple times, it is less efficient than needs to be. Instead, use the same iterator multiple times through Iterator::by_ref.
  7. I'd use expect in your main method, instead of matching and printing. There is a difference between the two, expect will cause a panic on Err, but that seems reasonable in main.
  8. I never use unwrap, instead preferring at least expect; this helps find the line when the code finally fails.
  9. Library code should very rarely panic; they should report errors. Especially report errors when it's because of input supplied to the code, panic for internal logic errors.
  10. The match in read_12h_time can be flattened; you can pattern match against Ok(10) directly.
  11. stdin doesn't need to be explicitly locked; read_line will lock it.
use std::io::{stdin, Error, ErrorKind, Result};

struct Time {
    hour: u8,
    minute: u8,
    second: u8,
}

impl Time {
    pub fn from_12h(time: &str) -> Result<Time> {
        assert!(time.chars().count() == 10);
        let mut chars = time.chars();

        let hour: String = chars.by_ref().take(2).collect();
        let minute: String = chars.by_ref().skip(1).take(2).collect();
        let second: String = chars.by_ref().skip(1).take(2).collect();
        let period: String = chars.by_ref().take(2).collect();

        Time::hour_from_12h_to_24h(hour.parse().unwrap(), &period).map(|hour_24h| {
            Time {
                hour: hour_24h,
                minute: minute.parse().unwrap(),
                second: second.parse().unwrap(),
            }
        })
    }

    fn hour_from_12h_to_24h(hour: u8, period: &str) -> Result<u8> {
        match period {
            "AM" => Ok(hour % 12),
            "PM" => Ok(hour % 12 + 12),
            _ => Err(Error::new(ErrorKind::InvalidData, format!("invalid period {}", period))),
        }
    }

    pub fn to_24h(&self) -> String {
        format!("{:02}:{:02}:{:02}", self.hour, self.minute, self.second)
    }
}

fn main() {
    let time_12h = read_12h_time().expect("input error");
    let time = Time::from_12h(&time_12h).expect("error");

    println!("{}", time.to_24h());
}

fn read_12h_time() -> Result<String> {
    let mut buffer = String::new();
    match read_line(&mut buffer) {
        Ok(10) => Ok(buffer),
        Ok(length) => {
            Err(Error::new(ErrorKind::InvalidInput,
                           format!("expected 10 characters, got {}", length)))
        }
        Err(e) => Err(e),
    }
}

fn read_line(buffer: &mut String) -> Result<usize> {
    stdin().read_line(buffer)
}

On to bigger issues:

  1. I don't like that the AM/PM matching and hour conversion are tangled together.
    • Instead, introduce an enumeration to handle the AM/PM.
  2. It feels like 12 hour time is a different type from 24 time.
    • Rename existing type to something unique.
    • Store the new AM/PM enumeration.
    • Introduce new type and add conversion methods.
  3. I don't like reusing io::Error for your own errors.
    • Introduce a custom error type. I like to use quick_error, but there are other solutions.
  4. I don't like that IO is embedded inside of the logic of the library.
    • Separate IO and parsing logic and only combine them at a higher level - in this case main.
    • Push time-specific logic (e.g. 10 character checks) back into the library. Note that this removes the duplication of the logic in the assert and removes a panic inside the library.
  5. The code doesn't fully participate in the Rust ecosystem.
    • Implement traits like FromStr and From and Display (perf!).
    • Derive standard traits to make it easier to use your types: #[derive(Debug, Copy, Clone, PartialEq)] is a good set.
  6. There are too many panics inside the library portion.
    • Fully utilize Result and try! or the try operator ?.
  7. There's too much unneeded allocation.
    • Instead of using characters and collecting them back to strings, operate on the incoming string and take slices instead.
    • Formatting can write directly into a formatter; there's no reason to allocate a string to just turn around and print it.
#[macro_use]
extern crate quick_error;

use std::str::FromStr;
use std::io::stdin;
use std::fmt;

quick_error! {
    #[derive(Debug, Clone, PartialEq)]
    enum Error {
        MissingMeridian {
            description("missing the meridian")
            display("Missing the meridian")
        }
        InvalidMeridian(what: String) {
            description("invalid meridian")
            display("Invalid meridian: {}", what)
        }
        MissingComponent(which: TimeComponent) {
            description("missing a time component")
            display("Missing {}", which)
        }
        InvalidComponent(which: TimeComponent, why: std::num::ParseIntError) {
            description("invalid time component")
            display("Invalid {}: {}", which, why)
            cause(why)
        }
    }
}

type Result<T> = std::result::Result<T, Error>;

#[derive(Debug, Copy, Clone, PartialEq)]
enum Meridian {
    AnteMeridian,
    PostMeridian,
}

#[derive(Debug, Copy, Clone, PartialEq)]
enum TimeComponent {
    Hour,
    Minutes,
    Seconds,
}

impl fmt::Display for TimeComponent {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        use TimeComponent::*;

        let name = match *self {
            Hour => "hour",
            Minutes => "minutes",
            Seconds => "seconds",
        };
        write!(f, "{}", name)
    }
}

impl FromStr for Meridian {
    type Err = Error;

    fn from_str(s: &str) -> Result<Meridian> {
        match s {
            "AM" => Ok(Meridian::AnteMeridian),
            "PM" => Ok(Meridian::PostMeridian),
            _ => Err(Error::InvalidMeridian(s.to_string())),
        }
    }
}

#[derive(Debug, Copy, Clone, PartialEq)]
struct TwelveHourTime {
    hour: u8,
    minutes: u8,
    seconds: u8,
    meridian: Meridian,
}

impl FromStr for TwelveHourTime {
    type Err = Error;

    fn from_str(time: &str) -> Result<TwelveHourTime> {
        use Error::*;
        use TimeComponent::*;

        let mut parts = time.splitn(3, ":");

        let hour = parts.next().ok_or(MissingComponent(Hour))?;
        let minutes = parts.next().ok_or(MissingComponent(Minutes))?;
        let seconds_meridian = parts.next().ok_or(MissingComponent(Seconds))?;

        if seconds_meridian.len() < 4 {
            return Err(MissingMeridian);
        }

        let (seconds, meridian) = seconds_meridian.split_at(2);

        let hour = hour.parse().map_err(|e| InvalidComponent(Hour, e))?;
        let minutes = minutes.parse().map_err(|e| InvalidComponent(Minutes, e))?;
        let seconds = seconds.parse().map_err(|e| InvalidComponent(Seconds, e))?;
        let meridian = meridian.parse()?;

        Ok(TwelveHourTime {
            hour: hour,
            minutes: minutes,
            seconds: seconds,
            meridian: meridian,
        })
    }
}

#[derive(Debug, Copy, Clone, PartialEq)]
struct TwentyFourHourTime {
    hour: u8,
    minutes: u8,
    seconds: u8,
}

impl From<TwelveHourTime> for TwentyFourHourTime {
    fn from(other: TwelveHourTime) -> Self {
        let hour = match other.meridian {
            Meridian::AnteMeridian => other.hour,
            Meridian::PostMeridian => other.hour + 12,
        };

        TwentyFourHourTime {
            hour: hour,
            minutes: other.minutes,
            seconds: other.seconds,
        }
    }
}

impl fmt::Display for TwentyFourHourTime {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "{:02}:{:02}:{:02}", self.hour, self.minutes, self.seconds)
    }
}

fn main() {
    let mut time = String::new();
    stdin().read_line(&mut time).expect("Unable to read input");

    let time: TwelveHourTime = time.trim().parse().expect("Unable to parse time");
    let time: TwentyFourHourTime = time.into();

    println!("{}", time);
}

The problem is very simple, but the code seems too lengthy?

I don't suppose I made the length any better! However, I feel that the final code is pretty efficient, and when you start taking error cases into account, I would disagree that the problem is inherently "simple". Parsing text is complicated. You can use tools like regular expressions or parser combinators / generators, but those are just abstractions - the complexity is just hidden from view.

There's still logical aspects here that are strange. For example, the original code does hours % 12, which seems wrong, and my modified code does not, which also seems wrong! It's probably better to throw an error for invalid hourly values.

\$\endgroup\$

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.