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

Ch4 isEven exercise does not mention sign #327

Closed
RhTDF opened this issue Apr 30, 2021 · 8 comments · Fixed by #339
Closed

Ch4 isEven exercise does not mention sign #327

RhTDF opened this issue Apr 30, 2021 · 8 comments · Fixed by #339

Comments

@RhTDF
Copy link

RhTDF commented Apr 30, 2021

The sample solution won't halt if given a negative integer as input. Perhaps the exercise specification should mention only positive integers as input.

@milesfrain
Copy link
Member

Yes, that's a good suggestion. Would you like to make a PR with that clarification?

We could also add a follow-up exercise that asks users to write an isEvenImproved which works on both positive and negative numbers, but that's a lot cleaner to write with pattern matching or guards as covered in chapter 5, rather than nested if/else.

@razcore-rad
Copy link

razcore-rad commented May 15, 2021

I'm not sure if it's a good idea to ask for an isEvenImproved cause as you mention, pattern matching is introduced later, and isEven already uses case pattern match in its implementation.

I'd just update it for negative numbers and be done with it:

isEven :: Int -> Boolean
isEven n = case n of
  0  -> true
  1  -> false
  _  -> isEven $ n + sign * 2
  where sign = if n < 0 then 1 else -1

What do you think @milesfrain? Should I PR this?

@milesfrain
Copy link
Member

isEven already uses case pattern match in its implementation.

That's a mistake, since case is introduced in Ch5.

So I'm thinking the best path forward here is to do the following:

  • Edit the exercise text to specify that this should only work for positive integers and that other chapters will cover other techniques for writing improved versions of this function (even if we don't actually revisit rewriting this specific function later).
  • Edit the solution to not use case.

Ideally, we'd restructure the book a bit using something like the following strategy:

  • Create a list of topics to cover.
  • Determine a beginner-friendly sequence to cover these topics in.
  • Ensure that no section or exercise relies on content from later chapters.

@razcore-rad
Copy link

That's true, I don't know why I thought of isEven as just a function that the student uses and the implementation doesn't matter. Previously I didn't read the book exercise, I just thought that it should be valid for negative integers as well since well, they can be either odd or even as well. So how about this solution instead?

isEven :: Int -> Boolean
isEven n
  = if sign * n < 2
      then n == 0
      else isEven $ n - sign * 2
  where sign = if n < 0 then -1 else 1

It uses only if statements and is good practice for the student too. It's a bit harder than just for natural numbers.

@gomain
Copy link

gomain commented May 16, 2021

Another one.

isEven :: Int -> Boolean
isEven i = isEven' 0 true
  where
    isEven' step isIt =
      if step == i
        then isIt
        else let nextStep = if i > 0 then step + 1 else step - 1
             in isEven' nextStep (not isIt)

@milesfrain
Copy link
Member

For reference, I think this was the original beginner-friendly solution that the exercise had in mind:

isEven :: Int -> Boolean
isEven n =
  if n == 0
    then true
    else not (isEven (n - 1))

I believe this is the simplest way to extend it to support negative inputs:

isEven :: Int -> Boolean
isEven n =
  if n == 0
    then true
    else if n < 0
      then isEven (-n)
      else not (isEven (n - 1))

It might be nice to revisit this isEven exercise in ch5's case section, although shadowing the ns gets a little weird:

isEven :: Int -> Boolean
isEven n = case n of
  0 -> true
  n | n < 0 -> isEven (-n)
  n -> not (isEven (n - 1))

Alternatives are:

isEven :: Int -> Boolean
isEven n = case n of
  0 -> true
  _ | n < 0 -> isEven (-n)
  _ -> not (isEven (n - 1))
// point free
isEven :: Int -> Boolean
isEven = case _ of
  0 -> true
  n | n < 0 -> isEven (-n)
  n -> not (isEven (n - 1))

Not sure which of the above is least confusing.

Side note, here's a version that's 2x as fast:

isEven :: Int -> Boolean
isEven n = case n of
  0 -> true
  1 -> false
  _ | n < 0 -> isEven (-n)
  _ -> isEven (n - 2)

We should also improve the test output.

Here's what we currently have:

In "Exercise Group - Recursion":
  In "Exercise - isEven":
    20 is even

I think this is more helpful:

In "Exercise Group - Recursion":
  In "Exercise - isEven":
    In "20 is even":
      expected true, got false

Opened #339 with some of these changes

@milesfrain
Copy link
Member

I forgot that this issue with isEven was already discussed pretty extensively in #303. I dropped the ball on following-up with a proposed fix at that time.

There are a few other issues with this sequence of exercises though - specifically countEven, which I hope we can remedy more effectively by following the recommendations of #340.

@gomain
Copy link

gomain commented May 17, 2021

The simplest way to extend the solution

isEven :: Int -> Boolean
isEven n =
  if n == 0
    then true
    else not (isEven (n - 1))

is to use abs. So

-- intentionally not point-free

isEven n = isPositiveEven (abs n)

isPositiveEven n = {- the original solution -}

All proposed solutions are variants of inverting the algorithm depending on sign or inverting the input had it been negative. Why not do it upfront?

-- not using `abs`
isEven n =
  if n < 0
    then isPositiveEven (-n)
    else isPositiveEven n
-- be explicit about algorithm
isEven n =
  if n < 0
    then isNegativeEven n
    else isPositiveEven n

In the spirit of be beginner-friendly, we can make the first exercise work on only positive integers, then follow by supporting negatives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants