4
\$\begingroup\$

I am getting started with rust. To get familiar with the language I implemented a datetime library: main.rs

mod datetime; use datetime::DateTime; use datetime::Date; use datetime::Time; fn main() { let date_time = DateTime::create(2022u16, 09, 17, 02, 53, 00); println!("Datetime: {}", date_time); println!("Date: {}", date_time.date); println!("Time: {}", date_time.time); let date = Date::new(2022u16, 09, 17); println!("Date: {}", date); let time = Time::new(03, 51, 00); println!("Time: {}", time); }

datetime.rs

use std::fmt; mod date; pub use self::date::Date; mod time; pub use self::time::Time; pub struct DateTime { pub date: Date, pub time: Time } impl DateTime { pub fn new(date: impl Into<Date>, time: impl Into<Time>) -> DateTime { DateTime { date: date.into(), time: time.into() } } pub fn create( year: impl Into<u16>, month: impl Into<u8>, day: impl Into<u8>, hour: impl Into<u8>, minute: impl Into<u8>, second: impl Into<u8> ) -> DateTime { DateTime::new(Date::new(year, month, day), Time::new(hour, minute, second)) } pub fn isoformat(&self) -> String { format!("{}T{}", self.date.isoformat(), self.time.isoformat()) } } impl fmt::Display for DateTime { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{}", self.isoformat()) } }

datetime/date.rs

use std::fmt; pub struct Date { pub year: u16, pub month: u8, pub day: u8 } impl Date { pub fn new(year: impl Into<u16>, month: impl Into<u8>, day: impl Into<u8>) -> Date { Date { year: year.into(), month: month.into(), day: day.into() } } pub fn isoformat(&self) -> String { format!("{:0>4}-{:0>2}-{:0>2}", self.year, self.month, self.day) } } impl fmt::Display for Date { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{}", self.isoformat()) } }

datetime/time.rs

use std::fmt; pub struct Time { pub hour: u8, pub minute: u8, pub second: u8 } impl Time { pub fn new(hour: impl Into<u8>, minute: impl Into<u8>, second: impl Into<u8>) -> Time { Time { hour: hour.into(), minute: minute.into(), second: second.into() } } pub fn isoformat(&self) -> String { format!("{:0>2}:{:0>2}:{:0>2}", self.hour, self.minute, self.second) } } impl fmt::Display for Time { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{}", self.isoformat()) } }

Since I'm completely new to rust, I'd like to have feedback on code style and proper use of language concepts.

\$\endgroup\$

1 Answer 1

Reset to default
2
\$\begingroup\$

welcome to the Rust community!

Your code is quite good. There are a couple areas where the code could be improved, but not by much. It would be helpful if you implement more code for review all at once, for example datetime parsing code.

DateTime::create

pub fn create( year: impl Into<u16>, month: impl Into<u8>, day: impl Into<u8>, hour: impl Into<u8>, minute: impl Into<u8>, second: impl Into<u8> )

The function name is not descriptive. It is idiomatic to name constructors e.g. with_date_and_time.

I do not like that your arguments are impl Into<u16> or impl Into<u8>. Why is it better to accept precisely u16 or u8 here? See the following error to figure out why. Very little convenience is lost -- the caller can do .into() if needed.

// error[E0283]: type annotations needed // --> src/main.rs:17:16 // | // 17 | let time = Time::new("03".parse().unwrap(), "51".parse().unwrap(), "00".parse().unwrap()); // | ^^^^^^^^^ cannot infer type for type parameter `impl Into<u8>` declared on the associated function `new` // | // = note: cannot satisfy `_: Into<u8>` // note: required by a bound in `Time::new` let time = Time::new("03".parse().unwrap(), "51".parse().unwrap(), "00".parse().unwrap()); println!("Time: {}", time);

Basically, impl Trait in argument position does not inform type inference. Some functions such as str::parse return generic results parameterized by a free parameter, so it may be best to guide inference.

Accepting impl Trait in argument position is usually unidiomatic.

Furthermore, impl Into<u16> is why you have to write 2022u16 and can't write just 2022.

So we have:

pub fn with_date_and_time( year: u16, month: u8, day: u8, hour: u8, minute: u8, second: u8 )

public fields

My recommendation is to add accessor methods and make fields private.

If you want to change inner representation someday, you wouldn't need to break backwards compatibility. This is important in library code. But in binary code, you may change representation without breaking other modules, so this is beneficial as well.

pub struct Time { hour: u8, minute: u8, second: u8 } impl Time { fn hour(&self) -> u8 { self.hour } // .. }

isoformat

I recommend a better method name such as to_iso_format or to_rfc3339. The name to_rfc3339 is consistent with chrono's DateTime.

more

Your code is not formatted with rustfmt. I recommend cargo fmt.

For lints, I recommend cargo clippy.

github

https://github.com/pczarn/codereview/tree/7a1952ec0bf9c5c813802becff27785ed57178de/2022/9/datetime

edit

Your impl Display allocates a String twice. It is better to implement isoformat that calls self.to_string() rather than the other way around.

derive Debug

Most library users expect types to implement Debug.

\$\endgroup\$

You must log in to answer this question.