Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iterators3 is missing limit tests and another error variant #1412

Closed
fleetingbytes opened this issue Mar 8, 2023 · 1 comment
Closed

iterators3 is missing limit tests and another error variant #1412

fleetingbytes opened this issue Mar 8, 2023 · 1 comment

Comments

@fleetingbytes
Copy link

fleetingbytes commented Mar 8, 2023

I compared my solution to @akhildevelops's solution and I think that most people go the same convenient way like him: handle the obvious couple of extra cases and then just return Ok(a / b). There is a case, however, where such careless division panics and we don't even test for it: 32::MIN / -1 (playground). It overflows the i32.

Hence, I argue, we should add this test case:

    #[test]
    fn test_overflow() {
        assert_eq!(divide(i32::MIN, -1), Err(DivisionError::IntegerOverflow)); // behold, a new DivisionError variant!
    }

We should not merely put this assertion in the test_not_divisible and return the NotDivisible error because i32::MIN is, of course, divisible by -1 . It's just that we have reached the limits of what rust can do with integer types. We need both a new test function and a new enum variant.

For good measure, we could add more assert_eq expressions to the rest of the test functions to test the limit values. (They would not overflow, but the ISTQB teaches to always include the limit values in your tests -- look what's happened if you don't!).

This playground presents the aforementioned additional IntegerOverflow variant and the first tests enriched with limit tests. It also shows my implementation of the divide function of which I'm not 100% sure that it is the most efficient one, but it passes the tests.

Discussion

An alternative way to correct iterators3 would be what I assume the author of the original iterators3 intended: Only consider non-negative integers. Their division never overflows. Well, then just use u32 if your exercise ignores half of the i32 value range anyway.

On the other hand, a signed integer division function makes a stronger case for using custom enums and structs for the error handling because there is one more error to deal with. Also the fact that the rustlings students would read the enum variant IntegerOverflow would remind them of this type of error and prompt them to deal with it in their implementation (and in their future carreers as programmers). They would also learn that it makes sense to include tests with limit values in their test suites.

The downside of having the signed integer division is that it shifts the exercise's focus from iterators to the intricacies of whole number division, potentially raising questions whether to use the checked_div or checked_div_euclid--how about yet another test case to make clear which one to use?

I personally have enjoyed the exercise. I learned that integer division is not as trivial as I thought and I was happy to see the exercise use custom errors because they really make sense more often than many budding programmers think. It motivated me to do a fair bit of digging in the standard library documentation of the Iterator trait. Learning about the usefulness of the FromIterator trait in conjunction with the collect function was a revelation--although I was lead there more by my own curiosity and comparison with other people's implementations than by the exercise itself.

Perhaps It would be best to make this whole thing a quiz and queue it after the rustling student has learned both about errors and iterators. Additionally, let him implement another division function which takes an iterator of dividends and an iterator of divisors (which may not be of equal lengths!) and let the function produce a collection of results. That would move the focus balance back to the iterators side again. The quiz could even go as far as having a moving window for a collection of divisors moving across a longer collection of dividends to produce a collection of collections of results, iterators within iterators.

@mo8it
Copy link
Contributor

mo8it commented Jul 8, 2024

Fixed in a4091ad by adding the error variant IntegerOverflow :)

@mo8it mo8it closed this as completed Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants