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

Adding IndexedLinkedList and its tests. #321

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

coderodde
Copy link

@coderodde coderodde commented Jul 13, 2022

Adding:

  • IndexedLinkedList.java
  • IndexedLinkedListTest.java
  • FingerListTest.java

Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

Hello,
What is the use case here compared to the existing CursorableLinkedList and NodeCachingLinkedList? Why would a user choose this 3rd implementation instead of one of the other two? I think we need a strong case to add yet another linked list and not confuse users.

* element (which requires \(\mathcal{O}(\sqrt{n})\) on evenly distributed
* finger lis).
*
* @author Rodion "rodde" Efremov
Copy link
Member

Choose a reason for hiding this comment

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

FYI, we do not use author tags. Attribution is usually done in changes.xml, and/or pom.xml.

*
* @author Rodion "rodde" Efremov
* @version 1.61 (Sep 26, 2021)
* @since 1.6 (Sep 1, 2021)
Copy link
Member

Choose a reason for hiding this comment

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

The next version of Collections is 4.5

* @version 1.61 (Sep 26, 2021)
* @since 1.6 (Sep 1, 2021)
*/
public class IndexedLinkedList<E> implements Deque<E>,
Copy link
Member

Choose a reason for hiding this comment

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

This should fit in the rest of the framework by subclassing AbstractLinkedList

import org.junit.Before;
import org.junit.Test;

public class FingerListTest {
Copy link
Member

Choose a reason for hiding this comment

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

Should extend AbstractLinkedListTest

Copy link
Author

Choose a reason for hiding this comment

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

Disagree. FingerList is not a conventional collection type, but rather an ad hoc list of fingers. Each FingerList is part of internals of IndexedLinkedList and is not exposed outside the ...list package.

Copy link
Member

@garydgregory garydgregory Jul 13, 2022

Choose a reason for hiding this comment

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

Disagree. FingerList is not a conventional collection type, but rather an ad hoc list of fingers. Each FingerList is part of internals of IndexedLinkedList and is not exposed outside the ...list package.

So why is it public?

import org.junit.Before;
import org.junit.Test;

public class IndexedLinkedListTest {
Copy link
Member

Choose a reason for hiding this comment

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

Should extend AbstractLinkedListTest

@garydgregory
Copy link
Member

-1: This code does not even compile.

The packaging is obviously wrong.

@coderodde
Copy link
Author

When I extend the AbstractLinkedList and implement Deque, a problem appears: Deque specifies void addFirst() and AbstractLinkedList boolean addFirst(), so it would seem that there is a compatibility issue.

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