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

Finished Chapter 1 #571

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Finished Chapter 1 #571

wants to merge 1 commit into from

Conversation

RCJamen
Copy link

@RCJamen RCJamen commented Jan 17, 2024

Solutions for Chapter 1

cc @vrom911 @chshersh

@RCJamen RCJamen requested a review from vrom911 as a code owner January 17, 2024 13:44
Copy link
Member

@vrom911 vrom911 left a comment

Choose a reason for hiding this comment

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

Very nice job on the Chapter! Congrats 🎉

@@ -449,7 +450,7 @@ Implement the function that takes an integer value and returns the next 'Int'.
function body with the proper implementation.
-}
next :: Int -> Int
next x = error "next: not implemented!"
next x = (x + 1 )
Copy link
Member

Choose a reason for hiding this comment

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

The brackets are not necessary here

Suggested change
next x = (x + 1 )
next x = x + 1

Comment on lines +498 to +499
| n `mod` 10 == 0 = 0
| otherwise = n `mod` 10
Copy link
Member

Choose a reason for hiding this comment

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

Nice! It could be simplified a bit though :)

This is actually could be combined into just one case, because in case of n mod 10 equal 0 it still returns 0 which is covered by just returning n mod 10.

While doing this simplification, you can notice that it actually could be just 1 case if you use abs function on the input 👌🏼

mid x y z
| (y <= x && x <= z) || (z <= x && x <= y) = x
| (x <= y && y <= z) || (z <= y && y <= x) = y
| (x <= z && z <= y) || (y <= z && z <= x) = z
Copy link
Member

Choose a reason for hiding this comment

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

As we mentioned, the compiler in Haskell is very attentive to the exhaustive pattern-matching. And here it would warn you that Pattern matching is not exhaustive, as the guards have quite complicated logic, and the compiler won't be able to prove that it covers all the cases.

Because of that, you will need to use another guard – | otherwise = ..., to tell the compiler, that your pattern matching is exhaustive 🙂

Suggested change
| (x <= z && z <= y) || (y <= z && z <= x) = z
| otherwise = z

Comment on lines +583 to +585
isVowel c
| elem c "aeiou" = True
| otherwise = False
Copy link
Member

Choose a reason for hiding this comment

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

Nice one! 👍🏼
As elem returns Bool itself, you can just return the result of this function:

isVowel c = elem c "aeiou"

One note in here, that sometimes, elem could be slower than the explicit pattern matching. I remember there were some benchmarks on one particular case, that showed how moving to pattern matching on each case separately drastically decrease time 🐎

Comment on lines +653 to +654
first = div last 10
second = mod last 10
Copy link
Member

Choose a reason for hiding this comment

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

That is a wonderful solution! 👏🏼 You correctly noticed that it is the div and mod, cool 😎

One hint to make your solution even shorter: you can see that you use both:

mod m 10
div m 10

The standard library has the divMod function, that actually combines inside both div and mod. And this is exactly what you use!.

So you could write it this way:

(x, y) = divMod m 10

You can see how we could pattern match on the pair 🙂

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

Successfully merging this pull request may close these issues.

2 participants