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 approaches for Leap #643

Merged
merged 14 commits into from
Jan 14, 2024
Merged

Add approaches for Leap #643

merged 14 commits into from
Jan 14, 2024

Conversation

ceddlyburge
Copy link
Contributor

@ceddlyburge ceddlyburge commented Jan 7, 2024

Add approaches for the leap exercise (which is being used as part of the 48 i 24 challenge, starting 16 January)
https://forum.exercism.org/t/48in24-exercise-01-16-leap/8964

Fixes #644.

@ceddlyburge ceddlyburge marked this pull request as ready for review January 8, 2024 08:50
Copy link
Contributor

@jiegillet jiegillet left a comment

Choose a reason for hiding this comment

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

This is really cool @ceddlyburge, thank you for taking the leap to add approaches (pun intended :D )

There is some cleaning to do in the code.

Using a case expression with a `Tuple` is idiomatic in Elm, but Tuple's have a maximum of 3 values, so can't be used for anything larger than this.

Strings and arrays can hold more values and can also be used with case expressions, which are useful in many circumstances.
Using an array with a case expression and recursion is especially common.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean an Array here, or a List?

divisbleBy number =
modBy number year == 0
in
if divisbleBy(400) then
Copy link
Contributor

Choose a reason for hiding this comment

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

The brackets are strange here

modBy number year == 0
in
if divisbleBy(400) then
return True
Copy link
Contributor

Choose a reason for hiding this comment

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

return?
Haha, the copy/paste is strong with this one :)
Where did you take the code from? Do they have the divisble typo as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha! I did copy paste this, but it was from an Elm solution on exercism, so I assumed that it passed the tests and worked!

else if divisbleBy(100) then
return False

else if divisbleBy(4) then
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that if something then True else False is ever better than something, especially in Elm where there is no truthiness.

@ceddlyburge
Copy link
Contributor Author

Thanks for the review Jeremie, I missed some pretty obvious things there! Hopefully they are all fixed now ...

Copy link
Contributor

@jiegillet jiegillet left a comment

Choose a reason for hiding this comment

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

One more thing to iron out, the rest looks good.


This approach uses exactly two tests to determine whether a year is a leap year.

The first test is for divisibility by 100.
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't match the actual code, since we start by checking 400.
From the description, the code would probably be (untested)

isLeapYear : Int -> Bool
isLeapYear year =
  let
    divisibleBy number = 
      modBy number year == 0 
  in
    if divisibleBy 100 then
      divisibleBy 400

    else
      divisibleBy 4

which only has one if statement. If you don't want to change the description, I would consider changing to this code (after double checking that it passes the tests).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does pass the tests, I'll use it ...

"Truth tables"
[esoteric-approach]
https://exercism.org/tracks/elm/exercises/leap/solutions/edgerunner
"An esoteric solution to leap, using recursion, a list and a case expression"
Copy link
Contributor

Choose a reason for hiding this comment

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

That really is esoteric 😆

Copy link
Contributor

@jiegillet jiegillet left a comment

Choose a reason for hiding this comment

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

Alright, looks good.
I didn't double check the config and stuff because I'm not familiar with it, but we can merge and see :)

@ceddlyburge ceddlyburge merged commit 639e59a into main Jan 14, 2024
6 checks passed
@ceddlyburge ceddlyburge deleted the cb/add-leap-approaches branch January 14, 2024 13:38
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.

Add approaches for leap
2 participants