Skip to content

Change pythagorean triplet function signature to remove confusion. #712

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

Closed
yaahc opened this issue Nov 12, 2018 · 2 comments
Closed

Change pythagorean triplet function signature to remove confusion. #712

yaahc opened this issue Nov 12, 2018 · 2 comments

Comments

@yaahc
Copy link

yaahc commented Nov 12, 2018

Right now Pythagorean triplet takes no arguments and expects students to encode in the function implementation assumptions about the solution they are trying to find, that the sum is 1000, and that it is a Pythagorean triplet. This is verified by a single test case that checks that the output is a specific number wrapped in an Option.

This specific problem doesn't justify the use of an Option type, students have to imagine that the problem could be changed and they could need to tweak their implementation such that it would have to search for a different sum that it could fail to find, which would justify returning None.

This leaves us in a situation where solutions that end in unreachable!() rather than returning None are technically valid, and students very frequently return Some(0) instead of None because there is never a negative test case, there cannot be with the current design.

My proposal is that we adjust the parameters so that the sum is passed as an argument. The documentation is updated to say, for example, there is one valid Pythagorean triplet that has a perimeter of 1000, rather than focusing the exercise on this specific Pythagorean triplet. And test cases are added for cases where the solution SHOULD fail to find a triplet.

Side issue: I'm not sure if there are any perimeter values at which there could be multiple pythagorean triplets, I'm guessing no. Also I expect there are certain problematic pythagorean triplets out there that might cause some solutions, such as those that rely on floating point math, to fail. Also I expect there are some that might look like valid triangles to incomplete solutions but actually be incorrect, particularly solutions where students use signed integers and don't correctly bound their iteration. I plan on setting up some tests to iterate over a large range of sums attempt to find problematic cases that might catch more implementation errors that are currently sneaking into review.

@yaahc
Copy link
Author

yaahc commented Nov 12, 2018

It may also be worth adjusting the return value to be a tuple of the sides, rather than a product, which just masks information about the solution.

@petertseng
Copy link
Member

I suggest that a way to solve this will be to follow the changes to the pythagorean-triplet problem that were enacted in exercism/problem-specifications#1332 . In such a case, the function in this track would become find(u32) -> Vec<(u32, u32, u32)> or similar.

It may also be necessary to update the README first, as exercism/problem-specifications#1211 directs.

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