Skip to main content
added 1 character in body
Source Link
SylvainD
  • 29.8k
  • 1
  • 49
  • 93

With the code provided, the only changes performed asare about blank lines and lines break.

With the code provided, the only changes performed as about blank lines and lines break.

With the code provided, the only changes performed are about blank lines and lines break.

Source Link
SylvainD
  • 29.8k
  • 1
  • 49
  • 93

Your code looks good to me! Here is how we can try to make things better.

Use more tools: fmt

Your code looks properly formatted. However, for your information, you can use cargo fmt to ensure that you never have to worry about formatting your code manually anymore.

With the code provided, the only changes performed as about blank lines and lines break.

Use more tools: clippy

clippy is a really nice tool to catch mistakes and improve your code.

In your case, there is nothing spectacular suggested but it is worth adding it to your toolbox.

Here are the suggestions, all of them are easy to take into account.

error: this argument is passed by value, but not consumed in the function body
 --> src/2021/day1/main_review.rs:6:23
  |
6 | fn parse_to_int(body: String) -> Vec<i32> {
  |                       ^^^^^^ help: consider changing the type to: `&str`
  |
  = note: `-D clippy::needless-pass-by-value` implied by `-D clippy::pedantic`
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value
error: single-character string constant used as pattern
 --> src/2021/day1/main_review.rs:9:16
  |
9 |         .split("\n")
  |                ^^^^ help: try using a `char` instead: `'\n'`
  |
  = note: `-D clippy::single-char-pattern` implied by `-D clippy::perf`
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_char_pattern
error: writing `&Vec` instead of `&[_]` involves a new object where a slice will do
  --> src/2021/day1/main_review.rs:12:21
   |
12 | fn part_one(parsed: &Vec<i32>) -> i32 {
   |                     ^^^^^^^^^ help: change this to: `&[i32]`
   |
   = note: `-D clippy::ptr-arg` implied by `-D clippy::style`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ptr_arg
error: casting `bool` to `i32` is more cleanly stated with `i32::from(_)`
  --> src/2021/day1/main_review.rs:18:23
   |
18 |         .map(|(a, b)| (b > a) as i32)
   |                       ^^^^^^^^^^^^^^ help: try: `i32::from(b > a)`
   |
   = note: `-D clippy::cast-lossless` implied by `-D clippy::pedantic`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#cast_lossless

Simplifying tests

In particular, once you've taken these suggestions into account, you can get rid of the String::from in your test cases and get something much more concise:

#[cfg(test)]
#[test]
fn test_int_conversion() {
    let res = parse_to_int("199\n200\n208\n210\n200\n207\n240\n269\n260\n263");
    assert_eq!(res, vec![199, 200, 208, 210, 200, 207, 240, 269, 260, 263]);
}

#[cfg(test)]
#[test]
fn test_part_one() {
    let res = parse_to_int("199\n200\n208\n210\n200\n207\n240\n269\n260\n263");
    let tot = part_one(&res);
    assert_eq!(tot, 7);
}

#[cfg(test)]
#[test]
fn test_part_two() {
    let res = parse_to_int("199\n200\n208\n210\n200\n207\n240\n269\n260\n263");
    let tot = part_two(&res);
    assert_eq!(tot, 5);
}

You can also try to define the input example just once (and maybe remove various variables used just once):

#[cfg(test)]
mod tests {
    use super::*;

    const INPUT_EXAMPLE: &str = "199\n200\n208\n210\n200\n207\n240\n269\n260\n263";

    #[cfg(test)]
    #[test]
    fn test_int_conversion() {
        assert_eq!(
            parse_to_int(INPUT_EXAMPLE),
            vec![199, 200, 208, 210, 200, 207, 240, 269, 260, 263]
        );
    }

    #[test]
    fn test_part_one() {
        assert_eq!(part_one(&parse_to_int(INPUT_EXAMPLE)), 7);
    }

    #[test]
    fn test_part_two() {
        assert_eq!(part_two(&parse_to_int(INPUT_EXAMPLE)), 5);
    }
}

Solution for part 1

The algorithm put in place for part 1 looks good to me.

However, instead of having parsed.iter() multiple times including one shifted of 1, you could directly yuse tuple_windows (from the itertools crate) to write: parsed.iter().tuple_windows().map(|(a, b)| i32::from(b > a)).sum().

This may look a bit overkilled but this will be even more relevant for the part 2.

Also, the variable name parsed does not convey much meaning. My preference would go for something like depths (even though it is easy to forget that we are tackling a submarine problem and not just a mathematical one).

As a final side note, instead of having map(|(a, b)| i32::from(b > a)).sum(), one could use filter(|(a, b)| a < b).count() but I cannot guarantee that it is more efficient and/or more idiomatic.

Solution for part 2

For this particular part, a mathematical trick can be applied. Indeed, we are comparing sum of sliding windows but doing it the obvious way: compute sliding windows, compute sums, compare sums. Instead, the following observation can be done:

Computing:

  • Window(n + 1) > Window(n)

ss the same as computing:

  • d(n+1) + d(n+2) + ... + d(n+1+windowsize) > d(n) + d(n+1) + ... + d(n+windowsize) (where d is depth)

Which is the same as computing:

  • d(n+1+windowsize) > d(n) which does not involve any kind of summation.

Hence, the solution for part 2 can be written:

fn part_two(depths: &[i32]) -> usize {
    // Window(n + 1) > Window(n)
    // d(n+1) + d(n+2) + ... + d(n+1+windowsize) > d(n) + d(n+1) + ... + d(n+windowsize)
    // d(n+1+windowsize) > d(n)
    depths
        .iter()
        .tuple_windows()
        .filter(|(a, _, _, d)| a < d)
        .count()
}