exercises(allergies): overhaul to use enum
#434
Conversation
| proc isAllergicTo*(allergies: Allergies, allergy: string): bool = | ||
| (allergies.score and 1 shl allergiesList.find(allergy)) != 0 | ||
| func isAllergicTo*(allergen: Allergen, score: int): bool = | ||
| (score and 1 shl allergen.ord) != 0 |
There was a problem hiding this comment.
I think we should use bitops.testbit here
There was a problem hiding this comment.
For real-life code, I'd agree. But by the same logic, the example.nim in this repo for the leap should just be
import times
proc isLeapYear*(y: int): bool {.inline.} =
times.isLeapYear(y)and the one for reverse-string should be
import algorithm, sugar
proc reverse*(s: string): string = # Yes, we might consider renaming this "reversed".
s.dup(algorithm.reverse)and so on. I think it's best for the example to have some flavor of what we want a user to submit - it should practice the intended techinque. For allergies, that's bit manipulation.
In general for the example solutions, there's a tension between:
- Maintainability: the shortest or simplest possible thing that passes the tests.
- Being idiomatic: the solution that we'd approve in a code review in the real world, when not in a performance hotspot.
- Being context-aware: the solution that we want a student to submit.
- Speed: some hyper-optimized solution that is allowed to overfit to the given test cases, use SIMD, etc, at the cost of greater complexity.
Where we want something that's:
- Idiomatic, but not importing so much that the solution doesn't really practice the intended technique.
- Fast, but without compromising readability, portability, or maintainability too much.
So, OK to leave as-is for now?
There was a problem hiding this comment.
I agree in the other 2 situations, but here, IMO, it's different. The point over here is to realize that you can just test the bit. I don't see it relevant in knowing how to test the bit with and or shifts. It still reveals an understanding of the fundamentals of the exercise and concepts.
Definitely non-blocking as its not user-facing code.
There was a problem hiding this comment.
I see your point - I'd agree that using bitops.testBit isn't "cheating" quite as much as e.g. times.isLeapYear. But I'd still prefer to inline all the logic - it's a bit fiddling exercise at heart, and there aren't that many of them.
Of course, the exercise is pretty artificial anyway. For pure Nim, if we want to make a bitset we don't create an int and twiddle bits - we just define an enum, use the built-in set and do things like allergies.incl Eggs.
Definitely non-blocking as its not user-facing code.
Yeah. And a goal for this PR was to keep the diff to the example minimal (although I muddied that because I think the cast is worth having - it's the exact demonstration of the exercise).
There was a problem hiding this comment.
I will still argue that testBit already encompasses the heart of the exercise. That shift is core. Either way I'm approving. I see your point
|
This PR is approved. @ee7 do you have any qualms about merging it? |
This exercise is intended to practice bit manipulation. Make it somewhat more idiomatic by avoiding the seq of strings. Of course, for Nim code in the real world, we should use the built-in `set` type. The overall design of this exercise hadn't changed since the initial implementation in 7e55686 ("Adds allergies exercise", 2018-04-17). Closes: 404
ee7
force-pushed
the
overhaul-allergies
branch
from
1273f81
to
fa97f32
Compare
|
Sorry for the delay. I just wanted to double-check everything, and rebase on |
Similar to previous discussions [1] I probably have a small preference for using shifts here, but it's fine either way. Note that bitops2 calls this `getBit` [2]. [1] exercism#434 (comment) [2] https://github.com/status-im/nim-stew/blob/3159137d9a31/stew/bitops2.nim#L420-L424 Suggested-by: ynfle
Closes: #404