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 topics to pascals-triangle #860

Merged
merged 2 commits into from
Oct 4, 2017
Merged

Conversation

puripant
Copy link

@puripant puripant commented Oct 4, 2017

Fixes #854


Reviewer Resources:

Track Policies

@puripant puripant changed the title Adding topics to pascals-triangle fixes #854 Add topics to pascals-triangle Oct 4, 2017
Copy link
Contributor

@FridaTveit FridaTveit left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for tackling this @puripant :)

I've left some comments, feel free to disagree with me or ask questions :)

config.json Outdated
"arrays",
"matrices",
"algorithms",
"mathematics"
Copy link
Contributor

Choose a reason for hiding this comment

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

Great suggestions! :) The only one I'm not sure about is recursion. You could quite easily solve this exercise without recursion I think, and I would hesitate to encourage people to use recusion in Java when they don't need to since using iteration is more suited to the language. What do you think? :)

I would also add exception_handling and integers as topics since the tests expect an exception to be thrown and it uses integers. Do you agree? :)

Also, all of these should be indented two spaces in from topics, like the entries for any other exercise with topics :)

Copy link
Author

Choose a reason for hiding this comment

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

I am fine with your suggestions. I will add exception_handling and integers as well. I guess it depends on whether you want the topic list to be more on the exhaustive side or not. I was even considering adding loops :)

Copy link
Contributor

@FridaTveit FridaTveit left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks @puripant! :)

@FridaTveit FridaTveit merged commit ee75eae into exercism:master Oct 4, 2017
@puripant puripant deleted the patch-1 branch October 4, 2017 18:09
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.

2 participants