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

Improve JoinedListIterator code coverage #1762

Merged
merged 1 commit into from
Feb 17, 2025

Conversation

Ivruix
Copy link
Contributor

@Ivruix Ivruix commented Feb 10, 2025

Added tests for the JoinedListIterator class.

One of the tests I wanted to add was the following:

@Test  
void previousIndexTest() {  
	final ListIterator<Integer> joined = new JoinedListIterator<>(  
		new ListOf<>(1).listIterator(),  
		new ListOf<>(2).listIterator(),  
		new ListOf<>(3).listIterator()  
	);  
	joined.next();  
	new Assertion<>(  
		"Must return index of the previous element",  
		joined.previousIndex(),  
		new IsEqual<>(0)  
	).affirm();  
	joined.next();
	new Assertion<>(  
		"Must return index of the previous element",  
		joined.previousIndex(),  
		new IsEqual<>(1)  
	).affirm();  
}

Surprisingly, the test failed. Currently, the previousIndex() method returns -1 for the first call and 0 for the second call, which contradicts the expected behavior. My suspision is that the implementation of previousIndex is incorrect.

@Override  
public int previousIndex() {  
	int previousidx = -1;  
	if (this.hasPrevious()) {  
		previousidx = this.cursor.get() - 1;
	}  
	return previousidx;  
}

The line previousidx = this.cursor.get() - 1 should likely be previousidx = this.cursor.get().

If my suspicion is confirmed, I will fix the implementation of previousIndex() and add the mentioned test to ensure the corrected behavior is validated.

@Ivruix
Copy link
Contributor Author

Ivruix commented Feb 11, 2025

@yegor256 please take a look

1 similar comment
@Ivruix
Copy link
Contributor Author

Ivruix commented Feb 17, 2025

@yegor256 please take a look

@yegor256 yegor256 merged commit 20597ed into yegor256:master Feb 17, 2025
13 checks passed
@yegor256
Copy link
Owner

@Ivruix thanks!

@Ivruix
Copy link
Contributor Author

Ivruix commented Feb 17, 2025

@yegor256 can you also please check whether I'm correct about previousIndex function? Off by one error like this might cause a lot of trouble for someone.

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