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
Merged

Add the accumulate problem #25

merged 2 commits into from Sep 28, 2015

Conversation

sturmer
Copy link

@sturmer sturmer commented Sep 22, 2015

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

@yurrriq
Copy link
Member

yurrriq 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)))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

or (arg . * . arg)

@yurrriq
Copy link
Member

yurrriq 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
Copy link
Member

yurrriq 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
Copy link
Author

sturmer 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) :-) )

Changed the body of two functions and added some tests.
@sturmer
Copy link
Author

sturmer 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
Copy link
Author

sturmer 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
Copy link
Member

yurrriq commented Sep 28, 2015

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

@arguello
Copy link
Contributor

looks good, thank you.

arguello added a commit that referenced this pull request Sep 28, 2015
Add the accumulate problem
@arguello arguello merged commit 649e228 into exercism:master Sep 28, 2015
@yurrriq
Copy link
Member

yurrriq commented Sep 28, 2015

🎉

@sturmer
Copy link
Author

sturmer commented Sep 29, 2015

Whooo thanks @arguello and @yurrriq!

@BNAndras BNAndras mentioned this pull request Sep 7, 2023
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.

None yet

4 participants