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

prime-factors: add canonical data #513

Merged
merged 1 commit into from
Jan 27, 2017

Conversation

bunnymatic
Copy link
Contributor

Added canonical-data.json for the prime factors exercise exercise description here.

Addresses https://github.com/exercism/todo/issues/129

@Insti
Copy link
Contributor

Insti commented Jan 25, 2017

Again I have some issues with the test descriptions.

Have a read through this comment: #451 (comment)

@@ -0,0 +1,62 @@
{
"for": {
"description": "returns prime factors for the given input number",
Copy link
Contributor

Choose a reason for hiding this comment

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

What are these 2 lines trying to achieve?

It's better to stick with the standard format. Have a look at some of the recent files added by @petertseng for good examples of what to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@Insti Insti Jan 26, 2017

Choose a reason for hiding this comment

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

Ok, Thanks. It's probably fine, further discussion here: #507 (review)

@bunnymatic
Copy link
Contributor Author

Agreed - the test descriptions are bad. And the note you pointed to was good.

I'm still having trouble figuring out exactly how to write the tests without the data in them and that indicate what we're testing. In fact, if i go that route, then I think we have far more tests than we need.

Thinking about it from scratch, I could imagine tests like

  • returns no factors for 1 which is prime
  • returns a single factor for a prime number (test 2 or 3)
  • returns multiple factors that are the same for a number that is a square of a prime (test 4 or 9)
  • returns multiple factors that are the same for a number that is a cube of a prime (test 8 or 27)
  • returns multiple prime factors for a value that is a product of primes and non-primes ( test 6 )
  • returns multiple prime factors for a value that is a product of primes
  • returns multiple prime factors for a value that is a product of primes even if the value is very large

If this were the test suite, we'd could probably lose the test for 2, 9 and 27 and 625 and still have the coverage needed to show that the method works.

Thoughts?

Insti
Insti previously approved these changes Jan 26, 2017
@Insti Insti dismissed their stale review January 26, 2017 09:01

Was trying to dismiss a request for changes.

@Insti
Copy link
Contributor

Insti commented Jan 26, 2017

returns no factors for 1 which is prime
returns a single factor for a prime number (test 2 or 3)
returns multiple factors that are the same for a number that is a square of a prime (test 4 or 9)
returns multiple factors that are the same for a number that is a cube of a prime (test 8 or 27)
returns multiple prime factors for a value that is a product of primes and non-primes ( test 6 )
returns multiple prime factors for a value that is a product of primes
returns multiple prime factors for a value that is a product of primes even if the value is very large

This sounds much better, you're describing what you are testing without worrying about the specifics of the actual numbers used, and have identified some redundant tests that do not need to be among the cases. 👍

@Insti
Copy link
Contributor

Insti commented Jan 26, 2017

Although the descriptions could be made less verbose, so they can be more easily used as test names.

'no factors'
'prime number'
...
'product of primes'

There's a bit of a discussion about descriptions and test names happening here: #506

@bunnymatic
Copy link
Contributor Author

bunnymatic commented Jan 26, 2017 via email

@bunnymatic bunnymatic force-pushed the prime-factors-add-canonical-data branch from 6afb3e0 to 6a9b757 Compare January 26, 2017 21:22
@bunnymatic
Copy link
Contributor Author

I can squish this down to 1 commit if need be, since the first commit is almost meaningless given the re-write.

{
"description" : "factors include a large prime",
"input" : 93819012551,
"expected" : [11, 9539, 894119]
Copy link
Contributor

Choose a reason for hiding this comment

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

caveat: I've not tried this problem myself recently.

Are these numbers (93819012551, 901255) within the bounds of reasonableness (<1sec) for today's computers without having to rely on Project Euler style optimisation trickery?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just completed this in elixir and ruby. it is interesting because a naive solution in ruby will take a very long time to complete. But there is an efficient solution that does not take too long (less than 1 sec i believe). I found it a good test to challenge the implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. Great 😄 Thanks.

@Insti
Copy link
Contributor

Insti commented Jan 26, 2017

Squashing is good.

Rewrite with shorter descriptions that speak to the functionality not the values in the tests - based on PR review discussions
@bunnymatic bunnymatic force-pushed the prime-factors-add-canonical-data branch from 73bda30 to 36faf10 Compare January 27, 2017 00:26
@bunnymatic
Copy link
Contributor Author

squashed

@Insti Insti merged commit d2b78e0 into exercism:master Jan 27, 2017
@Insti
Copy link
Contributor

Insti commented Jan 27, 2017

Thanks @bunnymatic ❤️

rpottsoh added a commit to rpottsoh/exercism-problem-specifications that referenced this pull request Jan 27, 2017
rpottsoh added a commit to rpottsoh/exercism-problem-specifications that referenced this pull request Jan 27, 2017
* prime-factors: add canonical data (exercism#513)

* etl: Add canonical data (exercism#507)

Add ETL exercise canonical data
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