4
\$\begingroup\$

The code, written in Rust, solves the Days in Month problem in CodeChef. It is also one of my first few programs written in Rust.

The code is accepted so that there are no logical mistakes. I am more interested in getting review feedback on style. I am not particularly happy with the code as is; it is rather bulky.

use std::io;

fn read_line(stdin: &io::Stdin) -> io::Result<String>
{
    let mut input = String::new();
    match stdin.read_line(&mut input)
    {
        Ok(_) => Ok(input.trim().to_string()),
        Err(e) => Err(e)
    }
}

fn to_day_of_week(input: &str) -> Option<usize>
{
    if input == "mon"
    {
        Some(0)
    }
    else if input == "tues"
    {
        Some(1)
    }
    else if input == "wed"
    {
        Some(2)
    }
    else if input == "thurs"
    {
        Some(3)
    }
    else if input == "fri"
    {
        Some(4)
    }
    else if input == "sat"
    {
        Some(5)
    }
    else if input == "sun"
    {
        Some(6)
    }
    else
    { 
        None
    }
}

pub fn codechef_nw1()
{
    let stdin = io::stdin();
    let read_first_line_result = read_line(&stdin);
    if read_first_line_result.is_ok()
    {
        let first_line = read_first_line_result.unwrap();
        let parse_first_line_result = first_line.parse::<i32>();
        if parse_first_line_result.is_ok()
        {
            let t = parse_first_line_result.unwrap();
            for _i in 0..t
            {
                let read_test_line_result = read_line(&stdin);
                if read_test_line_result.is_ok()
                {
                    let test_line = read_test_line_result.unwrap();
                    let find_space_result = test_line.find(' ');
                    if find_space_result.is_some()
                    {
                        let space_index = find_space_result.unwrap();
                        let test_token_1 = &test_line[..space_index];
                        let test_token_2 = &test_line[space_index + 1..];
                        let parse_day_result = test_token_1.parse::<i32>();
                        if parse_day_result.is_ok()
                        {
                            let days = parse_day_result.unwrap();
                            let day_of_week_result = to_day_of_week(test_token_2);
                            if day_of_week_result.is_some()
                            {
                                let mut day_of_week = day_of_week_result.unwrap();
                                let mut stats = [0, 0, 0, 0, 0, 0, 0];
                                for _d in 0..days
                                {
                                    stats[day_of_week] = stats[day_of_week] + 1;
                                    day_of_week = day_of_week + 1;
                                    day_of_week = day_of_week % 7;
                                }
                                println!("{} {} {} {} {} {} {}", stats[0], stats[1], stats[2], stats[3], stats[4], stats[5], stats[6]);
                            }                            
                        }
                    }
                }
            }
        }
    }
}
\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

to_day_of_week can be heavily simplified by using match instead of if-else:

fn to_day_of_week(input: &str) -> Option<usize> {
    match input {
        "mon" => Some(0),
        "tues" => Some(1),
        "wed" => Some(2),
        "thurs" => Some(3),
        "fri" => Some(4),
        "sat" => Some(5),
        "sun" => Some(6),
        _ => None,
    }
}

The bulkiness of your code can be somewhat helped if you provide additional functions and use ? instead of unwrap. Using unwrap is discouraged, as far as I know. If you don't want to use ? or additional functions, early returns prevent your code from getting too much indented.

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

We are certainly a bit far from clear, idiomatic Rust here. Let's start from the top.

The match statement in read_line can be made generic over the source type. It just needs to implement BufRead. But spoiler alert: we won't need it!

fn read_line<T: BufRead>(mut src: T) -> io::Result<String> {
    let mut input = String::new();
    match src.read_line(&mut input) {
        Ok(_) => Ok(input.trim().to_string()),
        Err(e) => Err(e),
    }
}

In to_day_of_week, match statements do work with strings:

fn to_day_of_week(input: &str) -> Option<usize> {
    match input {
        "mon" => Some(0),
        "tues" => Some(1),
        "wed" => Some(2),
        "thurs" => Some(3),
        "fri" => Some(4),
        "sat" => Some(5),
        "sun" => Some(6),
        _ => None
    }
}

There is one thing more that can be considered here, although not very important: usize is a bit too broad for a day-of-the-week identifier. We can make it explicit that this function returns a day of the week by defining a new type:

#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub enum DayOfWeek {
    Monday = 0,
    Tuesday,
    Wednesday,
    Thursday,
    Friday,
    Saturday,
    Sunday,
}

At this point we can also implement some utility functions and promote to_day_of_week to a static method.

impl DayOfWeek {

    pub fn from_offset(offset: usize) -> Option<Self> {
        use DayOfWeek::*;
        match offset {
            0 => Some(Monday),
            1 => Some(Tuesday),
            // and so on ...
            _ => None,
        }
    }

    pub fn from_text(input: &str) -> Option<Self> {
        use DayOfWeek::*;
        match input {
            "mon" => Some(Monday),
            "tues" => Some(Tuesday),
            // and so on ...
            _ => None,
        }
    }

    /// get the next day of the week
    pub fn next(self) -> Self {
        DayOfWeek::from_offset((self as usize + 1) % 7).unwrap()
    }
}

The current error handling code is very verbose and not very useful. Basically, you are silently skipping the following steps if any of the functions fail. Rather than if followed by unwrap, this would be more idiomatically achieved with pattern matching:

match read_line(x) {
    Ok(v) => {/* do things */}
    _ => {/* ignore */}
}

... which should also ring an alarm bell at this point. We are ignoring these errors completely, including I/O errors! Rust provides cleaner ways to escape from recoverable results, such as the ? operator. Let's first choose to enclose the code found in codechef_nw1 into a new function returning a Result (in this case a dynamic error type Box<Error> is fine). Then, we can replace all places where Results emerge with a ?. This should clear a lot of boilerplate. The first few lines would look something like this:

pub fn run_codechef_nw1() -> Result<(), Box<::std::error::Error>> {
    let stdin = io::stdin();
    let t = read_line(&stdin)?.parse::<i32>()?;
    for _ in 0..t {
// ...

When fetching the day of the week and the day in the month, splitting the two components from the string can be done with split. I chose to unwrap both tokens out of the Option, but in the event that your function needs to be robust against malformed input, you should conditionally return a custom error instead.

let mut tokens = test_line.split(' ');
let test_token_1 = tokens.next().unwrap();
let test_token_2 = tokens.next().unwrap();

Considering the new DayOfWeek type, we should now have:

let mut day_of_week = DayOfWeek::from_text(test_token_2).unwrap();

Array initialization can also be shortened with a different syntax:

let mut stats = [0; 7];

For the counting loop, we can do two things: (1) convert day_of_week to a usize with the as operator; (2) use the += operator for incrementing stats elements:

for _ in 0..days {
    stats[day_of_week as usize] += 1;
    day_of_week = day_of_week.next();
}

At the end of your function, don't forget

    Ok(())
}

The main entry function may then do something like this:

run_codechef_nw1().unwrap();

We can also make our run_codechef_nw1 functions generic over any source of data by making it take some type which implements BufRead.

pub fn codechef_nw1<T: BufRead>(mut stdin: T) -> Result<(), Box<::std::error::Error>> {
    let t = read_line(&mut stdin)?.parse::<i32>()?;
// ...

Note that the input source needs to be passed by mutable reference to other functions, since we don't want it to be moved after the first read. With this done, we can even hardcode our own test inputs.

fn main() {
    // fake input data
    let input_data = br#"2
28 mon
31 wed"#;

    let stdin = Cursor::new(input_data);
    run_codechef_nw1(stdin).unwrap();
}

Using iterators

Rather than read_line, working with an iterator of lines will often lead to nicer code. With a small little twist at the beginning of the function, we can fetch and handle incoming lines with the iterator API:

let mut lines = stdin.lines();
let t = lines.next().unwrap()?.parse::<i32>()?;
for line in lines.take(t) { // consider the first `t` lines only
    let test_line = line?; // handle line reading error
// ...

As usual in Rust, don't forget to call rustfmt over your code to keep it formatted according to community conventions.

Here's the full solution in the Playground.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Enum variants would commonly use PascalCase instead of SCREAMING_SNAKE_CASE. You could also consider implementing FromStr instead of the bespoke from_text. If you really wanted to be cutting-edge, you could use TryFrom instead of from_offset. It's micro-optimization, but I'd probably use a u8 instead of a usize. I'm not a fan of using the discriminant to allow rotating through the values, but it works. \$\endgroup\$
    – Shepmaster
    Commented Jan 2, 2018 at 18:37
  • \$\begingroup\$ @Shepmaster I propose we call it STEPPED_ON_SNAKE_CASE. \$\endgroup\$
    – Zeta
    Commented Jan 2, 2018 at 18:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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