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

Does InputStream need to be home-made? #17

Open
ashleyfrieze opened this issue Nov 13, 2020 · 1 comment
Open

Does InputStream need to be home-made? #17

ashleyfrieze opened this issue Nov 13, 2020 · 1 comment

Comments

@ashleyfrieze
Copy link
Contributor

Please see PR #16 in which I replaced the implementation of the input stream with something from the default Java API. I don't see why the home-made implementation is better, but it has a surprising effect, which is the reason I've raised this PR.

Why does the original version of the unit test work? The first scanner should, on nextLine, fast forward through the text to cache some of the line after? In the original code it doesn't, meaning that the test is able to pass. However, I think the test should not pass (it doesn't with this refactor) and the replacement test is more correct.

I also changed the README where I found it slightly hard to understand why you were asserting the second line, but it seemed simpler when we assert both first and second line.

This may or may not seem like an improvement. Just sharing.

@ashleyfrieze ashleyfrieze changed the title InputStream does not need to be home-made Does InputStream need to be home-made? Nov 14, 2020
@ashleyfrieze
Copy link
Contributor Author

I've found some use cases where the default implementation starts to struggle with my expectations... so I've written some tests in #18 that pass with the current implementation... but should they? Or should Scanner be hitting the exception thrown at the end of the stream earlier?

If the stream throws when it's asked to read beyond its end, then the tests in #18 would not all pass, because scanner reads ahead. For interest, merge both branches together to see what I mean.

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

No branches or pull requests

1 participant