HiveBrain v1.2.0
Get Started
← Back to all entries
patternrustMinor

Converting a 12h clock to 24h clock

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
converting24h12hclock

Problem

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 {
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::().unwrap(), period) {
Ok(hour_24h) =>
Ok(Time {
hour: hour_24h,
minute: minute.parse::().unwrap(),
second: second.parse::().unwrap()
}),
Err(e) => Err(e),
}
}

fn hour_from_12h_to_24h(hour : u8, period : String) -> Result {
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!

Solution

-
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


-
Style issues - learn to love rustfmt:

  • There's no space before : when specifiying a type on a variable.



  • Use trailing commas.



  • Match arms longer than one line should use curly braces {}



-
Accept a &str instead of String; the function doesn't need to take ownership of the string.

  • There's no need for the turbofish (::<>) on the parse calls; type inference handles that just fine.



  • 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.



  • 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.



  • 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.



  • I never use unwrap, instead preferring at least expect; this helps find the line when the code finally fails.



  • 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.



  • The match in read_12h_time can be flattened; you can pattern match against Ok(10) directly.



  • 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 {
        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 {
        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 {
    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 {
    stdin().read_line(buffer)
}


On to bigger issues:

  • I don't like that the AM/PM matching and hour conversion are tangled together.



  • Instead, introduce an enumeration to handle the AM/PM.



  • 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.



  • 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.



  • 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.



  • 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.



  • There are too many panics inside the library portion.



  • Fully utilize Result and try! or the try operator ?.



  • There's too much unneeded allocation.



  • Instead of using characters and collecting them back to st

Code Snippets

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)
}
#[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 {
   

Context

StackExchange Code Review Q#149476, answer score: 5

Revisions (0)

No revisions yet.