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

Replace nondeterministic performantTraversables #97

Merged
merged 2 commits into from Dec 10, 2017

Conversation

FRosner
Copy link
Member

@FRosner FRosner commented Nov 25, 2017

The performantTraversables exercise has three problems:

  1. It is not deterministic. People report (and I experienced it myself) that the result is sometime true and sometimes false.
  2. You should not benchmark method calls like this. The JVM needs to warm up, there are a lot of optimizations going on. Use a microbenchmarking framework.
  3. The comment states that you might receive a StackOverflowError. This is not true, as reduceRight is implemented based on reduceLeft, making it also tail recursive if reduceLeft is tail recursive.

I suggest to replace the exercise with another one demonstrating the relationship between the left and the right operation. The new exercise is deterministic. Also I removed the wrong comment about the StackOverflowError.

The `performantTraversables` exercise has three problems:

1. It is not deterministic. People report (and I experienced it myself) that the result is sometime `true` and sometimes `false`.
2. You should not benchmark method calls like this. The JVM needs to warm up, there are a lot of optimizations going on. Use a microbenchmarking framework.
3. The comment states that you might receive a `StackOverflowError`. This is not true, as `reduceRight` is implemented based on `reduceLeft`, making it also tail recursive if `reduceLeft` is tail recursive.

I suggest to replace the exercise with another one demonstrating the relationship between the left and the right operation. The new exercise is deterministic. Also I removed the wrong comment about the `StackOverflowError`.
Copy link
Member

@raulraja raulraja left a comment

Choose a reason for hiding this comment

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

@FRosner thanks for your contribution.

@raulraja raulraja merged commit b363a4d into scala-exercises:master Dec 10, 2017
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

2 participants