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.

Sign up for GitHub

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

Jump to bottom

exercises(allergies): overhaul to use enum #434

Merged
merged 1 commit into from
Jan 11, 2023
Merged

exercises(allergies): overhaul to use enum #434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 13 additions & 21 deletions exercises/practice/allergies/.meta/example.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,16 @@
import sequtils

type
Allergies* = object
score*: int

var
allergiesList = [
"eggs",
"peanuts",
"shellfish",
"strawberries",
"tomatoes",
"chocolate",
"pollen",
"cats"
]

Allergen* = enum
Eggs
Peanuts
Shellfish
Strawberries
Tomatoes
Chocolate
Pollen
Cats

proc isAllergicTo*(allergies: Allergies, allergy: string): bool =
(allergies.score and 1 shl allergiesList.find(allergy)) != 0
func isAllergicTo*(score: int, allergen: Allergen): bool =
(score and 1 shl allergen.ord) != 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use bitops.testbit here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Maintainability: the shortest or simplest possible thing that passes the tests.
  2. Being idiomatic: the solution that we'd approve in a code review in the real world, when not in a performance hotspot.
  3. Being context-aware: the solution that we want a student to submit.
  4. 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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@ee7 ee7 Oct 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


proc lst*(allergies: Allergies): seq[string] =
allergiesList.filterIt(allergies.isAllergicTo(it))
func allergies*(score: int): set[Allergen] =
cast[typeof(result)](score)
155 changes: 52 additions & 103 deletions exercises/practice/allergies/test_allergies.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,212 +3,161 @@ import allergies

suite "Eggs allergy":
test "not allergic to anything":
let allergies = Allergies(score: 0)
check allergies.isAllergicTo("eggs") == false
check isAllergicTo(0, Eggs) == false

test "allergic only to eggs":
let allergies = Allergies(score: 1)
check allergies.isAllergicTo("eggs") == true
check isAllergicTo(1, Eggs)

test "allergic to eggs and something else":
let allergies = Allergies(score: 3)
check allergies.isAllergicTo("eggs") == true
check isAllergicTo(3, Eggs)

test "allergic to something, but not eggs":
let allergies = Allergies(score: 2)
check allergies.isAllergicTo("eggs") == false
check isAllergicTo(2, Eggs) == false

test "allergic to everything":
let allergies = Allergies(score: 255)
check allergies.isAllergicTo("eggs") == true
check isAllergicTo(255, Eggs)

suite "Peanuts allergy":
test "not allergic to anything":
let allergies = Allergies(score: 0)
check allergies.isAllergicTo("peanuts") == false
check isAllergicTo(0, Peanuts) == false

test "allergic only to peanuts":
let allergies = Allergies(score: 2)
check allergies.isAllergicTo("peanuts") == true
check isAllergicTo(2, Peanuts)

test "allergic to peanuts and something else":
let allergies = Allergies(score: 7)
check allergies.isAllergicTo("peanuts") == true
check isAllergicTo(7, Peanuts)

test "allergic to something, but not peanuts":
let allergies = Allergies(score: 5)
check allergies.isAllergicTo("peanuts") == false
check isAllergicTo(5, Peanuts) == false

test "allergic to everything":
let allergies = Allergies(score: 255)
check allergies.isAllergicTo("peanuts") == true
check isAllergicTo(255, Peanuts)

suite "Shellfish allergy":
test "not allergic to anything":
let allergies = Allergies(score: 0)
check allergies.isAllergicTo("shellfish") == false
check isAllergicTo(0, Shellfish) == false

test "allergic only to shellfish":
let allergies = Allergies(score: 4)
check allergies.isAllergicTo("shellfish") == true
check isAllergicTo(4, Shellfish)

test "allergic to shellfish and something else":
let allergies = Allergies(score: 14)
check allergies.isAllergicTo("shellfish") == true
check isAllergicTo(14, Shellfish)

test "allergic to something, but not shellfish":
let allergies = Allergies(score: 10)
check allergies.isAllergicTo("shellfish") == false
check isAllergicTo(10, Shellfish) == false

test "allergic to everything":
let allergies = Allergies(score: 255)
check allergies.isAllergicTo("shellfish") == true
check isAllergicTo(255, Shellfish)

suite "Strawberries allergy":
test "not allergic to anything":
let allergies = Allergies(score: 0)
check allergies.isAllergicTo("strawberries") == false
check isAllergicTo(0, Strawberries) == false

test "allergic only to strawberries":
let allergies = Allergies(score: 8)
check allergies.isAllergicTo("strawberries") == true
check isAllergicTo(8, Strawberries)

test "allergic to strawberries and something else":
let allergies = Allergies(score: 28)
check allergies.isAllergicTo("strawberries") == true
check isAllergicTo(28, Strawberries)

test "allergic to something, but not strawberries":
let allergies = Allergies(score: 20)
check allergies.isAllergicTo("strawberries") == false
check isAllergicTo(20, Strawberries) == false

test "allergic to everything":
let allergies = Allergies(score: 255)
check allergies.isAllergicTo("strawberries") == true
check isAllergicTo(255, Strawberries)

suite "Tomatoes allergy":
test "not allergic to anything":
let allergies = Allergies(score: 0)
check allergies.isAllergicTo("tomatoes") == false
check isAllergicTo(0, Tomatoes) == false

test "allergic only to tomatoes":
let allergies = Allergies(score: 16)
check allergies.isAllergicTo("tomatoes") == true
check isAllergicTo(16, Tomatoes)

test "allergic to tomatoes and something else":
let allergies = Allergies(score: 56)
check allergies.isAllergicTo("tomatoes") == true
check isAllergicTo(56, Tomatoes)

test "allergic to something, but not tomatoes":
let allergies = Allergies(score: 40)
check allergies.isAllergicTo("tomatoes") == false
check isAllergicTo(40, Tomatoes) == false

test "allergic to everything":
let allergies = Allergies(score: 255)
check allergies.isAllergicTo("tomatoes") == true
check isAllergicTo(255, Tomatoes)

suite "Chocolate allergy":
test "not allergic to anything":
let allergies = Allergies(score: 0)
check allergies.isAllergicTo("chocolate") == false
check isAllergicTo(0, Chocolate) == false

test "allergic only to chocolate":
let allergies = Allergies(score: 32)
check allergies.isAllergicTo("chocolate") == true
check isAllergicTo(32, Chocolate)

test "allergic to chocolate and something else":
let allergies = Allergies(score: 112)
check allergies.isAllergicTo("chocolate") == true
check isAllergicTo(112, Chocolate)

test "allergic to something, but not chocolate":
let allergies = Allergies(score: 80)
check allergies.isAllergicTo("chocolate") == false
check isAllergicTo(80, Chocolate) == false

test "allergic to everything":
let allergies = Allergies(score: 255)
check allergies.isAllergicTo("chocolate") == true
check isAllergicTo(255, Chocolate)

suite "Pollen allergy":
test "not allergic to anything":
let allergies = Allergies(score: 0)
check allergies.isAllergicTo("pollen") == false
check isAllergicTo(0, Pollen) == false

test "allergic only to pollen":
let allergies = Allergies(score: 64)
check allergies.isAllergicTo("pollen") == true
check isAllergicTo(64, Pollen)

test "allergic to pollen and something else":
let allergies = Allergies(score: 224)
check allergies.isAllergicTo("pollen") == true
check isAllergicTo(224, Pollen)

test "allergic to something, but not pollen":
let allergies = Allergies(score: 160)
check allergies.isAllergicTo("pollen") == false
check isAllergicTo(160, Pollen) == false

test "allergic to everything":
let allergies = Allergies(score: 255)
check allergies.isAllergicTo("pollen") == true
check isAllergicTo(255, Pollen)

suite "Cats allergy":
test "not allergic to anything":
let allergies = Allergies(score: 0)
check allergies.isAllergicTo("cats") == false
check isAllergicTo(0, Cats) == false

test "allergic only to cats":
let allergies = Allergies(score: 128)
check allergies.isAllergicTo("cats") == true
check isAllergicTo(128, Cats)

test "allergic to cats and something else":
let allergies = Allergies(score: 192)
check allergies.isAllergicTo("cats") == true
check isAllergicTo(192, Cats)

test "allergic to something, but not cats":
let allergies = Allergies(score: 64)
check allergies.isAllergicTo("cats") == false
check isAllergicTo(64, Cats) == false

test "allergic to everything":
let allergies = Allergies(score: 255)
check allergies.isAllergicTo("cats") == true
check isAllergicTo(255, Cats)

suite "List the allergies":
test "no allergies":
let allergies = Allergies(score: 0)
check allergies.lst.len == 0
check allergies(0) == {}

test "just eggs":
let allergies = Allergies(score: 1)
check allergies.lst == @["eggs"]
check allergies(1) == {Eggs}

test "just peanuts":
let allergies = Allergies(score: 2)
check allergies.lst == @["peanuts"]
check allergies(2) == {Peanuts}

test "just strawberries":
let allergies = Allergies(score: 8)
check allergies.lst == @["strawberries"]
check allergies(8) == {Strawberries}

test "eggs and peanuts":
let allergies = Allergies(score: 3)
check allergies.lst == @["eggs", "peanuts"]
check allergies(3) == {Eggs, Peanuts}

test "more than eggs but not peanuts":
let allergies = Allergies(score: 5)
check allergies.lst == @["eggs", "shellfish"]
check allergies(5) == {Eggs, Shellfish}

test "lots of stuff":
let allergies = Allergies(score: 248)
check allergies.lst == @["strawberries", "tomatoes", "chocolate",
"pollen", "cats"]
check allergies(248) == {Strawberries, Tomatoes, Chocolate, Pollen, Cats}

test "everything":
let allergies = Allergies(score: 255)
check allergies.lst == @["eggs", "peanuts", "shellfish", "strawberries",
"tomatoes", "chocolate", "pollen", "cats"]
check allergies(255) == {Eggs, Peanuts, Shellfish, Strawberries, Tomatoes,
Chocolate, Pollen, Cats}

test "no allergen score parts":
let allergies = Allergies(score: 509)
check allergies.lst == @["eggs", "shellfish", "strawberries", "tomatoes",
"chocolate", "pollen", "cats"]
check allergies(509) == {Eggs, Shellfish, Strawberries, Tomatoes, Chocolate,
Pollen, Cats}

test "no allergen score parts without highest valid score":
let allergies = Allergies(score: 257)
check allergies.lst == @["eggs"]
check allergies(257) == {Eggs}