Currently you get a pass for a scenario that you shouldn’t. Example with my code:
// GetItem retrieves an item from a slice at given position.
// If the index is out of range, we want it to return -1.
func GetItem(slice []int, index int) int {
if index < 0 {
return -1
}
if index >= len(slice) {
return -1
}
return slice[index]
}
// SetItem writes an item to a slice at given position overwriting an existing value.
// If the index is out of range the value needs to be appended.
func SetItem(slice []int, index, value int) []int {
if GetItem(slice, index) != -1 {
slice[index] = value
return slice
}
return append(slice, value)
}
If the slice contains the value -1 at the index it will always append. I can’t be the only person who did this.
I also posted this as an issue on github but it was auto-closed. I don’t think it should have been.
Fair.
I bring it up because I think there’s a lot of value in being able to understand the shape of a problem. It’s always good practice to think about edge cases. This is a learning platform afterall and people will come here with different ability levels.
Exercism is a large group of volunteers and two staff members, spread across hundreds of repos. We have opted to use the forum as our primary way to track issues over having issues scattered across hundreds of repos. As such, issues on GitHub are automatically closed and redirected to where we do discuss issues: the forum.
This case wouldn’t directly teach about slices but it does catch a logic error where a student might think GetItem() can be used for bounds checking and reuse the wrong function. Code reuse here is a good idea, but it would need to be code that actually does bounds checking; GetItem() looks like it might do that but it doesn’t actually do that.
This is a good catch of an interesting corner case. I think it’s worth adding a test for this. I can do ahead and add that edge case. Or, if you prefer, you’re welcome to open a PR to add a test case for this.
Is this a concept exercise? Since the web editor for concepts doesn’t show the tests, I think this might be too confusing to get an unexpected failure.
Yeah, that’s my hesitation as well. It’s a concept exercise I recall doing earlier in the syllabus before errors were introduced. Idiomatic Go would be return an error alongside the value being returned. If the error is nil, the value is safe to use, and if the error isn’t nil, don’t use the value. Since we can’t use errors here, we can’t distinguish between -1 representing a possible item in the slice or an index out of range for the slice.
Yes, it’s a concept exercise. I’ll ensure that test failures are explicit about what’s going on, eg SetItem([]int{0, -1, -2}, 1, 5) = []int{0, -1, -2, 5}, want []int{0, -5 -2}.
Yes, the GetItem() returning -1 for errors is not idiomatic Go. I’m not sure that precludes us from testing SetItem([]int{0, -1, -2}, 1, 5). There’s no indication or suggestion that students should use GetItem([]int{0, -1, -2}, 1) to check if 1 is a valid index. A clean test should make the issue pretty clear here. I don’t think it would be confusing… but I could certainly be wrong!
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.
[no important files changed]
For more information, refer to the [documentation](https://exercism.org/docs/building/tracks#h-avoiding-triggering-unnecessary-test-runs). If you are unsure whether to add the message or not, please ping `@exercism/maintainers-admin` in a comment. Thank you!
I would imagine you don’t want to retest every student?