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

Checking the validity of input ListIterators #259

Closed

Conversation

emopers
Copy link

@emopers emopers commented Jul 19, 2019

Partition.java calls of j.previous() on java.util.ListIterator j and i.next() on java.util.ListIterator i without checking if there are any elements to iterate over. Because the method is public and the iterators are obtained from inputs, they could be invalid (e.g., an empty list). This could lead to an exception. This pull request adds a 'hasNext()' and 'hasPrevious()' check. * This PR is a sequel of PR #236 *

@ctrueden
Copy link
Member

Thanks @emopers!

However, after seeing the same sorts of changes in #236 and #260, I am beginning to think this sort of change might become annoying to people reading the tests. Adding these tests to every place where cursors are used bogs down the logic of the tests. Properly, we should be testing the cursor iteration in its own test method, and then in other tests, we can assume the iteration works properly.

In other words: it is not the job of every test to test every piece of logic used in the execution of that test.

@emopers Did you find all these places using some code linting tool? I'm not sure I agree that the lack of hasNext checks in these circumstances are "violations"...

@tpietzsch What do you think? Should we merge #236, #259 and #260? Or close them without merge?

@tpietzsch
Copy link
Member

@ctrueden I would close #259 and #260 without merge.
I agree with the intention of #236, but it should rather check i.nextIndex() < j.previousIndex() or something (I would need to think about the exact condition). Unfortunately, there is no way to check whether i and j are iterators into the same list, which is also an expectation about the input.

@ctrueden ctrueden closed this Aug 16, 2019
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