Conversation
I've had a mentee copy the "normalize" function from the test code. I didn't notice at first and explained it's unidiomatic to use a match statement only for the guards, because it's just a weird way of writing an if-else-statement. This version isn't an if-else-statement (which would've been fine too), but I find it the most readable. I have confirmed that the generated assemply is essentially the same, i.e. this doesn't generate _two_ comparison instructions.
|
This PR touches files which potentially affect the outcome of the tests of an exercise. This will cause all students' solutions to affected exercises to be re-tested. If this PR does not affect the result of the test (or, for example, adds an edge case that is not worth rerunning all tests for), please add the following to the merge-commit message which will stops student's tests from re-running. Please copy-paste to avoid typos. For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping |
ellnix
left a comment
There was a problem hiding this comment.
Looks fine, unfortunate to find out you were calling out our own code 😆
|
As a side note, can we discuss what's going on with the example.rs of this exercise? It seems to be working by accident, there is no backtracking logic whatsoever. If you take the backtrack test and simply put the first domino at the end in the input, the solution fails: #[test]
#[ignore]
fn need_backtrack() {
// (2, 4) used to be at the end, which is used as the first domino in example.rs, causing it to pass this test by accident
// The solution simply grabs the first domino it can find, starting from the smallest values, and since it starts with (4, 2) it doesn't run into the problem of hitting (2, 3) before (2, 4) and needing to backtrack
let input = &[(2, 3), (3, 1), (2, 4), (2, 4), (1, 2)];
let output = dominoes::chain(input);
assert!(output.is_some());
assert_correct(input, output.unwrap());
} |
|
Huh, strange. I guess we can replace the example with a sound solution. I considered if the tests need to be improved to cover this somehow. But I don't think there's value in adding tests against strange and random edge cases like "the solution reads the first domino from the back, but the rest in order". |
|
@senekor It's stranger than that: it doesn't even look at the dominoes in order. Instead, it effectively searches for a matching domino starting from the lowest number on the insignificant side. I'm not sure reading the first domino from the end is strange, I imagine most devs would reach for some |
It was discovered that the previous example solution doesn't perform any backtracking and only passes the relevant test accidentally: #2127 (comment)
It was discovered that the previous example solution doesn't perform any backtracking and only passes the relevant test accidentally: #2127 (comment)
I've had a mentee copy the "normalize" function from the test code. I didn't notice at first and explained it's unidiomatic to use a match statement only for the guards, because it's just a weird way of writing an if-else-statement.
This version isn't an if-else-statement (which would've been fine too), but I find it the most readable. I have confirmed that the generated assemply is essentially the same, i.e. this doesn't generate two comparison instructions.
[no important files changed]