patternrustMinor
Converting a 12h clock to 24h clock
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 (
My own first impressions:
```
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!
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
matchconstructs, 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
-
Style issues - learn to love rustfmt:
-
Accept a
On to bigger issues:
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 theparsecalls; type inference handles that just fine.
- Become very familiar with all the methods on
OptionandResultandIterator. These are the bread and butter of idiomatic, concise, and performant Rust code. Specifically, if youmatchon aResultand have a match arm forOkandErr, it's likely you want to useResult::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
expectin yourmainmethod, instead of matching and printing. There is a difference between the two,expectwill cause a panic onErr, but that seems reasonable inmain.
- I never use
unwrap, instead preferring at leastexpect; 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_timecan be flattened; you can pattern match againstOk(10)directly.
stdindoesn't need to be explicitly locked;read_linewill 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::Errorfor 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
assertand removes a panic inside the library.
- The code doesn't fully participate in the Rust ecosystem.
- Implement traits like
FromStrandFromandDisplay(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
Resultandtry!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.