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.

Sign up for GitHub

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

(#1230) ListIteratorOf can be mutable and immutable, depending on origin #1231

Merged
merged 2 commits into from
Nov 12, 2019
Merged

(#1230) ListIteratorOf can be mutable and immutable, depending on origin #1231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
52758f3
(#1230) ListIteratorOf can be mutable and immutable, depending on origin
fanifieiev Nov 9, 2019
9a4929c
(#1230) Added todo to change the some lists to mutable behavior as well
fanifieiev Nov 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 5 additions & 13 deletions src/main/java/org/cactoos/list/ListIteratorOf.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,13 @@
import org.cactoos.scalar.Unchecked;

/**
* Iterator of the list that doesn't allow mutations.
* List Iterator of the list that allows mutations if the original
* list iterator does support.
*
* <p>There is no thread-safety guarantee.
*
* @param <T> Items type
* @since 0.35
* @todo #1219:30min {@link ListIteratorOf} does not have to be immutable.
* Make it possible to use mutable operations like remove/set/add in this
* {@link ListIteratorOf} and change the class javadoc as well.
*/
public final class ListIteratorOf<T> implements ListIterator<T> {

Expand Down Expand Up @@ -112,22 +110,16 @@ public int previousIndex() {

@Override
public void remove() {
throw new UnsupportedOperationException(
"Iterator is read-only and doesn't allow removing items"
);
this.origin.value().remove();
}

@Override
public void set(final T item) {
throw new UnsupportedOperationException(
"Iterator is read-only and doesn't allow rewriting items"
);
this.origin.value().set(item);
}

@Override
public void add(final T item) {
throw new UnsupportedOperationException(
"Iterator is read-only and doesn't allow adding items"
);
this.origin.value().add(item);
}
}
6 changes: 6 additions & 0 deletions src/main/java/org/cactoos/list/package-info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,11 @@
* Lists, tests.
*
* @since 0.14
* @todo #1230:30min The following list implmenetation classes
* {@link org.cactoos.list.Joined}, {@link org.cactoos.list.Mapped},
* {@link org.cactoos.list.NoNulls}, {@link org.cactoos.list.Shuffled},
* {@link org.cactoos.list.Solid}, {@link org.cactoos.list.Solid},
* {@link org.cactoos.list.Sorted}, {@link org.cactoos.list.Sticky},
* {@link org.cactoos.list.Synced} should support mutability.
*/
package org.cactoos.list;
18 changes: 9 additions & 9 deletions src/test/java/org/cactoos/list/ListEnvelopeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
* That's because this test should check the original behavior of ListEnvelope
* Now this test checks behavior of the Immutable decorator
*/
@SuppressWarnings({ "PMD.TooManyMethods", "PMD.AvoidDuplicateLiterals" })
@SuppressWarnings({"PMD.TooManyMethods", "PMD.AvoidDuplicateLiterals"})
public final class ListEnvelopeTest {

@Test(expected = UnsupportedOperationException.class)
Expand Down Expand Up @@ -159,9 +159,9 @@ public void returnsSubListWithUnsupportedAdd() {
}

@Test
public void getsPreviousIndex() {
public void mustReturnPreviousIndex() {
new Assertion<>(
"List iterator returns incorrect previous index",
"List Iterator must return previous index",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@fanifieiev why is ListIteratorOf tested in ListEnvelopeTest? I think those tests should be in their own file (and removed from here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@victornoel I agree with your concern, I just left them where they were previously.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@fanifieiev so please move them in their own file to properly identify what is being tested

new ListIteratorOf<>(
new ListOf<>(1)
).previousIndex(),
Expand All @@ -170,9 +170,9 @@ public void getsPreviousIndex() {
}

@Test
public void getsPrevious() {
public void mustReturnPreviousElement() {
new Assertion<>(
"List iterator returns incorrect previous item",
"List Iterator must return previous element",
new ListIteratorOf<>(
new ListOf<>(3, 7),
1
Expand All @@ -182,9 +182,9 @@ public void getsPrevious() {
}

@Test
public void getsNextIndex() {
public void mustReturnNextIndex() {
new Assertion<>(
"List iterator returns incorrect next index",
"List iterator must return next index",
new ListIteratorOf<>(
new ListOf<>(1)
).nextIndex(),
Expand All @@ -193,9 +193,9 @@ public void getsNextIndex() {
}

@Test
public void getsNext() {
public void mustReturnNextElement() {
new Assertion<>(
"List iterator returns incorrect next item",
"List iterator must return next item",
new ListIteratorOf<>(
new ListOf<>(5, 11, 13),
1
Expand Down
95 changes: 57 additions & 38 deletions src/test/java/org/cactoos/list/ListIteratorNoNullsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,13 @@
*/
package org.cactoos.list;

import java.util.ArrayList;
import java.util.List;
import java.util.ListIterator;
import java.util.NoSuchElementException;
import org.junit.Test;
import org.llorllale.cactoos.matchers.Assertion;
import org.llorllale.cactoos.matchers.ScalarHasValue;
import org.llorllale.cactoos.matchers.Throws;

/**
Expand All @@ -38,12 +42,15 @@
public final class ListIteratorNoNullsTest {

@Test
public void getThrowsErrorIfListIteratorNextValueIsNullValue() {
public void mustThrowsErrorIfListIteratorNextValueIsNull() {
new Assertion<>(
"must throw error if removed value is null",
() -> new ListIteratorNoNulls<>(
new ListOf<>(null, 2, 3).listIterator()
).next(),
"must throw error next item is null",
() -> {
new ListIteratorNoNulls<>(
new ListOf<>(null, 2, 3).listIterator()
).next();
return 0;
},
new Throws<>(
"Next item is NULL",
IllegalStateException.class
Expand All @@ -52,13 +59,17 @@ public void getThrowsErrorIfListIteratorNextValueIsNullValue() {
}

@Test
public void getThrowsErrorIfListIteratorPreviousValueIsNullValue() {
final ListIterator<Integer> listiterator =
new ListOf<>(null, 2, 3).listIterator();
listiterator.next();
public void mustThrowsErrorIfListIteratorPreviousValueIsNull() {
new Assertion<>(
"must throw error if previous value is null",
() -> new ListIteratorNoNulls<>(listiterator).previous(),
() -> {
new ListIteratorNoNulls<>(
new ListOf<>(
null, 2, 3
).listIterator(1)
).previous();
return 0;
},
new Throws<>(
"Previous item is NULL",
IllegalStateException.class
Expand All @@ -67,53 +78,61 @@ public void getThrowsErrorIfListIteratorPreviousValueIsNullValue() {
}

@Test
public void addThrowsErrorForImmutableListIterator() {
public void mustAddToListIterator() {
new Assertion<>(
"must throw error if modified with add",
"must add to list iterator",
() -> {
new ListIteratorNoNulls<>(
new ListOf<>(1, 2, 3).listIterator()
).add(4);
return 0;
final List<Integer> list = new ArrayList<>(2);
list.add(1);
list.add(2);
final ListIterator<Integer> iterator = new ListIteratorNoNulls<>(
list.listIterator()
);
iterator.next();
iterator.add(4);
return iterator.previous();
},
new Throws<>(
"Iterator is read-only and doesn't allow adding items",
UnsupportedOperationException.class
)
new ScalarHasValue<>(4)
).affirm();
}

@Test
public void removeThrowsErrorForImmutableListIterator() {
public void mustRemoveFromListIterator() {
new Assertion<>(
"must throw error if modified with remove",
"must remove element from list iterator",
() -> {
new ListIteratorNoNulls<>(
new ListOf<>(1, 2, 3).listIterator()
).remove();
return 0;
final List<Integer> list = new ArrayList<>(2);
list.add(1);
list.add(2);
final ListIterator<Integer> iterator = new ListIteratorNoNulls<>(
list.listIterator()
);
iterator.next();
iterator.remove();
return iterator.previous();
},
new Throws<>(
"Iterator is read-only and doesn't allow removing items",
UnsupportedOperationException.class
NoSuchElementException.class
)
).affirm();
}

@Test
public void setThrowsErrorForImmutableListIterator() {
public void mustSetValueListIterator() {
new Assertion<>(
"must throw error if modified with set",
"must set element into list iterator",
() -> {
new ListIteratorNoNulls<>(
new ListOf<>(1, 2, 3).listIterator()
).set(4);
return 0;
final List<Integer> list = new ArrayList<>(2);
list.add(1);
list.add(2);
final ListIterator<Integer> iterator = new ListIteratorNoNulls<>(
list.listIterator()
);
iterator.next();
iterator.set(4);
return iterator.previous();
},
new Throws<>(
"Iterator is read-only and doesn't allow rewriting items",
UnsupportedOperationException.class
)
new ScalarHasValue<>(4)
).affirm();
}
}
3 changes: 0 additions & 3 deletions src/test/java/org/cactoos/list/NoNullsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,6 @@ public void addThrowsErrorForImmutableListIterator() {
return 0;
},
new Throws<>(
"Iterator is read-only and doesn't allow adding items",
UnsupportedOperationException.class
)
).affirm();
Expand All @@ -167,7 +166,6 @@ public void removeThrowsErrorForImmutableListIterator() {
return 0;
},
new Throws<>(
"Iterator is read-only and doesn't allow removing items",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@fanifieiev why remove those parts of the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@victornoel If you look at the test and the following stacktrace, you can see that the exception is thrown by the collection returned by Collections.unmodifiableList, that is why it throws exception with no message.
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

@fanifieiev I better understand, thank you, but why is ListIteratorNoNulls based on an immutable version of ListIterator? I think that it is coherent we the changes we are making that ListIteratorNoNulls is to be only concerned with nulls, not with mutability. Please fix it or add a todo to continue the work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@victornoel I did not catch your concern. The ListIteratorOf I changed is not based on the immutable version of ListIterator.
Could you provide details regarding you concern?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm talking about ListIteratorNoNulls: those tests on which we are currently commenting seem to validate that it is immutable but I don't think it is in the spirit of the changes that are being done to the ListIterator type hierarchy in this PR: either change ListIteratorNoNulls to be coherent with the changes you are currently making, or add a todo in order for someone to do it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@fanifieiev I think we spent enough time on this PR, please add todos and let ARC validate them by merging.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@fanifieiev one todo is enough, no need to multiplicate them btw :)

Copy link
Contributor Author

@fanifieiev fanifieiev Nov 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@victornoel There is one thing that concerns me.
Let's say we will make those mentioned classes mutable. Let's take Sorted as an example:
List<Integer> sorted = new Sorted<>(2, 1, 0, 5);
Sorted list is now sorted.
Let's add one more element:
sorted.add(1);
Sorted list is not sorted anymore.
You got the idea of my concern?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@fanifieiev that's why you should leave a todo and let the next job performer think about this problem. Maybe we will conclude that Sorted can't be mutable, but that's out of scope of the present issue. Personally I believe the real problem is that it's not relevant to decorate collection to sort them, but that's only my opinion ;) Also, nothing prevent to implement Sorted using a Java ordered list and implement insertion in a sorted manner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@victornoel Ok, I have just pushed one more commit with 'todo'. Please have a look.

UnsupportedOperationException.class
)
).affirm();
Expand All @@ -182,7 +180,6 @@ public void setThrowsErrorForImmutableListIterator() {
return 0;
},
new Throws<>(
"Iterator is read-only and doesn't allow rewriting items",
UnsupportedOperationException.class
)
).affirm();
Expand Down