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 the accumulate problem #25

Merged
merged 2 commits into from Sep 28, 2015

Conversation

Projects
None yet
4 participants
@sturmer
Copy link

commented Sep 22, 2015

Getting my feet wet. Any feedback would be appreciated :)

sturmer
@yurrriq

This comment has been minimized.

Copy link
Member

commented Sep 22, 2015

I think the general idea is to avoid built-in functions like map.

You could implement accumulate using pattern-matching like this:

(define/match (accumulate proc lst)
  [(proc '())          '()]
  [(proc `(,x . ,rst)) `(,(proc x) . ,(accumulate proc rst))])

or more traditionally like this:

(define (accumulate proc lst)
  (if (null? lst)
      '()
      (cons (proc (car lst))
            (accumulate proc (cdr lst)))))
empty)

(test-equal? "doubling"
(accumulate '(1 3 4) (lambda (arg) (arg * arg)))

This comment has been minimized.

Copy link
@yurrriq

yurrriq Sep 22, 2015

Member

I'm assuming you meant (* arg arg) here.

application: not a procedure;
 expected a procedure that can be applied to arguments
  given: 1
  arguments...:
   #<procedure:*>
   1
  context...:
   accumulate

This comment has been minimized.

Copy link
@bennn

bennn Sep 23, 2015

Contributor

or (arg . * . arg)

"accumulate tests"

(test-equal? "empty list"
(accumulate empty (lambda (arg) (arg)))

This comment has been minimized.

Copy link
@yurrriq

yurrriq Sep 22, 2015

Member

The (arg) in the body of the lambda here, would attempt to call arg as a function with no arguments, which surely isn't your intention.

@yurrriq

This comment has been minimized.

Copy link
Member

commented Sep 22, 2015

It would probably be helpful to have a couple more tests. You might find some inspiration in the LFE version.

@yurrriq

This comment has been minimized.

Copy link
Member

commented Sep 22, 2015

Sorry if my comments come off as terse or overly critical. That's not my intention. It's late here and I shouldn't be coding any more tonight...

Thanks very much for getting involved! I hope my feedback is constructive rather than discouraging.

@sturmer

This comment has been minimized.

Copy link
Author

commented Sep 22, 2015

Hi @yurrriq no worries, your comments are very constructive and I'll address them carefully later today (it was very early here and I missed silly stuff like (* arg arg) instead of (arg * arg) :-) )

sturmer
Address reviews
Changed the body of two functions and added some tests.
@sturmer

This comment has been minimized.

Copy link
Author

commented Sep 24, 2015

I have addressed the immediate concerns. I am trying to reproduce also the last test from LFE but I am still a 3-year-old when it comes to list comprehensions (I am using this chance to learn that too). Thanks again for your comments @yurrriq and @bennn, please go ahead if you have other comments.

@sturmer

This comment has been minimized.

Copy link
Author

commented Sep 28, 2015

Hi, anyone has any other comments? If the initial comments are addressed, I'd be happy to know how I can help to have it merged.

@yurrriq

This comment has been minimized.

Copy link
Member

commented Sep 28, 2015

Without running the code it looks good to me. @arguello, will you check it out and merge?

@arguello

This comment has been minimized.

Copy link
Member

commented Sep 28, 2015

looks good, thank you.

arguello added a commit that referenced this pull request Sep 28, 2015

Merge pull request #25 from sturmer/master
Add the accumulate problem

@arguello arguello merged commit 649e228 into exercism:master Sep 28, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@yurrriq

This comment has been minimized.

Copy link
Member

commented Sep 28, 2015

🎉

@sturmer

This comment has been minimized.

Copy link
Author

commented Sep 29, 2015

Whooo thanks @arguello and @yurrriq!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.