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

Don't Merge - HW 1 #2

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

Don't Merge - HW 1 #2

wants to merge 3 commits into from

Conversation

adkron
Copy link
Collaborator

@adkron adkron commented Nov 19, 2018

This is my solution for the first week. I put notes in the commit messages so check those out. I thought the best way to talk about the code would be through PRs, but never merge them. I look forward to being torn up.

This is the first homework that I've done. The hardest part was going through
docs and trying to find out what functions are available to me. I did know of
the existence of the compose operator, but I didn't know what it was. After I
found it in the docs I was able to make a the code more succinct. I think this
is what they talk about with functional programming. I made the program about
functions.

I also implemented the hw in Elixir. I don't know how I feel about it there, but
I did like that I had map_every.

Amos King @adkron <[email protected]>
Hanoi 3 is optimal, but hanoi 4 is not. I was wondering if there is a way to use
hanoi 3 in hanoi 1. I spent quite a bit of time trying to figure that out.

Amos King @adkron <[email protected]>
@adkron adkron requested a review from WizardOfOgz November 19, 2018 20:11
@glguy
Copy link

glguy commented Nov 19, 2018

I only have minor comments. The Haskell looks just fine to me.

I'd avoid having three cases for hanoi and merge the 1 and n cases:

-- | Generate list of instructions for moving a tower of disks from one peg to
-- another given a extra storage peg.
hanoi ::
  Integer  {- ^ tower height -} ->
  a        {- ^ source peg   -} ->
  a        {- ^ target peg   -} ->
  a        {- ^ extra  peg   -} ->
  [(a, a)] {- ^ move list    -}
hanoi 0 _ _ _ = []
hanoi n s t x = hanoi (n-1) s x t
             ++ [(s,t)]
             ++ hanoi (n-1) x t s

I'm generally opposed to using type synonyms as documentation, but I know that that was just the way that the exercise was specified. I went one step further from unfolding the type signatures and generalized the type signature.

Another minor point is that extra parentheses when building function composition should generally be elided, so I'd recommend the following edit:

- next = hanoi . (subtract 1)
+ next = hanoi . subtract 1

Using cycle and zipWith together is a nice approach for avoiding the need for explicit recursion there, nice!

@glguy
Copy link

glguy commented Nov 19, 2018

Another bit of code if you wanted to fuse doubleEveryOther and sum together in order to process the list of digits in one pass could look like this:

-- | Sum a list of numbers while doubling every other element in the list
-- starting from the last element and working backward. The last element
-- is not doubled and the elements alternate from there.
doubleEveryOtherSum :: Num a => [a] -> a
doubleEveryOtherSum = go 0 0
  where
    go acc1 acc2 (x:xs) = go (x + acc2) acc1 xs
    go acc1 acc2 []     = 2 * acc2 + acc1

I wanted to make sure I was reusing as much code as I could.

Amos King @adkron <[email protected]>
hanoi 1 s t _ = [(s, t)]
hanoi n s t a = next n s a t ++
hanoi 1 s t a ++
next n a t s
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm still having to adjust to the order of operations in Haskell, since it wasn't immediately clear how this was working.

The fact that all multi-argument functions are actually implemented as a series of one-argument functions is just ingenious...even if it does throw me for a loop sometimes. Anyway, nice use of it here!


doubleEveryOther :: [Integer] -> [Integer]
doubleEveryOther xs = xs
doubleEveryOther = flipMap (zipWith (*) (cycle [1, 2]))
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 for a concise and elegant solution. And for knowing which tools you have in the toolbox (zipWith, cycle).

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.

3 participants