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

add soln to ch1 #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

add soln to ch1 #1

wants to merge 2 commits into from

Conversation

InfiniteVerma
Copy link
Owner

Solutions for Chapter 1

cc @vrom911 @chshersh

Copy link

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

Amazing work on the Chapter 👏🏼 🎉

src/Chapter1.hs Outdated
@@ -568,7 +573,8 @@ True
>>> isVowel 'x'
False
-}
isVowel c = error "isVowel: not implemented!"
isVowel :: Char -> Bool
isVowel c = c `elem` ['a', 'e', 'i', 'o', 'u']
Copy link

Choose a reason for hiding this comment

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

Nice one! 👍🏼

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 🐎

src/Chapter1.hs Outdated
Comment on lines 667 to 668
| mod n 10 == n = n
| otherwise = firstDigit $ div n 10
Copy link

Choose a reason for hiding this comment

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

I see, that for negative numbers this implementation will not give a correct result.
However, this is easy to fix with your solution 👍🏼

Copy link
Owner Author

Choose a reason for hiding this comment

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

I see. I thought all tests were passing since nothing was showing in red while in fact, this was leading to an infinite loop 😅

Copy link

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

Looks fantastic, ready to be merged 🚢

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