[jira] [Comment Edited] (COLLECTIONS-310) Modifications of a SetUniqueList.subList() invalidate the parent list

["Thomas Vahrst (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/COLLECTIONS-310?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13563537#comment-13563537 ] 

Thomas Vahrst edited comment on COLLECTIONS-310 at 1/26/13 4:32 PM:
--------------------------------------------------------------------

As already stated in the javadoc of SetUniqueList, this class does not adhere to the api contract of java.util.List. The class name as well as the implemented interface (java.util.List) implies the behavior of a List, but that is not true. Several methods violate the List contract and state this in its javadocs. 

Furthermore: some javadocs are not clear enough about the contract violation. For example the javadoc of the 'add()' method states a 'violation' of the api because the method may return 'false'. But this method also violates the java.util.List *semantics*, because the specified element is not only added, an other element is possibly removed during the invocation. 

Imagine a generic sort algorithm for lists, which internal moves elements in the list by *first* adding the element at the new position and then removing the element at the old position. This algorithm would probably remove most of the elements in the list... (Btw: this test fails with an UnsupportedOperationException:
{code}
  List<String> list = new ArrayList<String>();
  list.addAll( Arrays.asList("one", "two", "three", "four", "five", "six", "seven", "eight", "nine", "ten"));
  SetUniqueList<String> uniquelist = SetUniqueList.setUniqueList(list);

  Collections.sort(uniquelist);

{code}

In my opinion this jira issue concerning the subList() behavior cannot be solved in a way that makes the behavior expectable for the user. So instead of providing an implementation which may work in most cases but sometimes fails and describing this strange behavior (it took some time to understand the issue...) in the method's javadoc, I would suggest to throw an UnsupportedOperationException.

I personally would consider removing the whole class. Many developers use the apache commons classes as code reference for their own concrete coding problems and SetUniqueList isn't a good example of the decorator pattern - as stated above. (And in addition violates the liskov substitution principle). The better solution is already described in SetUniqueList's javadoc:

{code}
 * The {@link org.apache.commons.collections.set.ListOrderedSet ListOrderedSet}
 * class provides an alternative approach, by wrapping an existing Set and
 * retaining insertion order in the iterator.
{code}

What do yout think? Any comments?
                
      was (Author: t.vahrst):
    As already stated in the javadoc of SetUniqueList, this class does not adhere to the api contract of java.util.List. The class name as well as the implemented interface (java.util.List) implies the behavior of a List, but that is not true. Several methods violate the List contract and state this in its javadocs. 

Furthermore: some javadocs are not clear enough about the contract violation. For example the javadoc of the 'add()' method states a 'violation' of the api because the method may return 'false'. But this method also violates the java.util.List *semantics*, because the specified element is not only added, an other element is possibly removed during the invocation. 

Imagine a generic sort algorithm for lists, which internal moves elements in the list by *first* adding the element at the new position and then removing the element at the old position. This algorithm would probably remove most of the elements in the list... (Btw: this test fails with an UnsupportedOperationException:
{code}
  List<String> list = new ArrayList<String>();
  list.addAll( Arrays.asList("one", "two", "three", "four", "five", "six", "seven", "eight", "nine", "ten"));
  SetUniqueList<String> uniquelist = SetUniqueList.setUniqueList(list);

{code}

In my opinion this jira issue concerning the subList() behavior cannot be solved in a way that makes the behavior expectable for the user. So instead of providing an implementation which may work in most cases but sometimes fails and describing this strange behavior (it took some time to understand the issue...) in the method's javadoc, I would suggest to throw an UnsupportedOperationException.

I personally would consider removing the whole class. Many developers use the apache commons classes as code reference for their own concrete coding problems and SetUniqueList isn't a good example of the decorator pattern - as stated above. (And in addition violates the liskov substitution principle). The better solution is already described in SetUniqueList's javadoc:

{code}
 * The {@link org.apache.commons.collections.set.ListOrderedSet ListOrderedSet}
 * class provides an alternative approach, by wrapping an existing Set and
 * retaining insertion order in the iterator.
{code}

What do yout think? Any comments?
                  
> Modifications of a SetUniqueList.subList() invalidate the parent list
> ---------------------------------------------------------------------
>
>                 Key: COLLECTIONS-310
>                 URL: https://issues.apache.org/jira/browse/COLLECTIONS-310
>             Project: Commons Collections
>          Issue Type: Bug
>          Components: List
>    Affects Versions: 3.2, Nightly Builds
>            Reporter: Christian Semrau
>            Priority: Minor
>             Fix For: 4.0
>
>
> The List returned by SetUniqueList.subList() is again a SetUniqueList. The contract for List.subList() says that the returned list supports all the operations of the parent list, and it is backed by the parent list.
> We have a SetUniqueList uniqueList equal to {"Hello", "World"}. We get a subList containing the last element. Now we add the element "Hello", contained in the uniqueList but not in the subList, to the subList.
> What should happen?
> Should the subList behave like a SetUniqueList and add the element - meaning that it changes position in the uniqueList because at the old place it gets removed, so now uniqueList equals {"World", "Hello"} (which fails)?
> Or should the element not be added, because it is already contained in the parent list, thereby violating the SetUniqueList-ness of the subList (which fails)?
> I prefer the former behaviour, because modifications should only be made through the subList and not through the parent list (as explained in List.subList()).
> What should happen if we replace (using set) the subList element "World" with "Hello" instead of adding an element?
> The subList should contain only "Hello", and for the parent list, the old element 0 (now a duplicate of the just set element 1) should be removed (which fails).
> And of course the parent list should know what happens to it (specifically, its uniqueness Set) (which fails in the current snapshot).
> 	public void testSubListAddNew() {
> 		List uniqueList = SetUniqueList.decorate(new ArrayList());
> 		uniqueList.add("Hello");
> 		uniqueList.add("World");
> 		List subList = uniqueList.subList(1, 2);
> 		subList.add("Goodbye");
> 		List expectedSubList = Arrays.asList(new Object[] { "World", "Goodbye" });
> 		List expectedParentList = Arrays.asList(new Object[] { "Hello", "World", "Goodbye" });
> 		assertEquals(expectedSubList, subList);
> 		assertEquals(expectedParentList, uniqueList);
> 		assertTrue(uniqueList.contains("Goodbye")); // fails
> 	}
> 	public void testSubListAddDuplicate() {
> 		List uniqueList = SetUniqueList.decorate(new ArrayList());
> 		uniqueList.add("Hello");
> 		uniqueList.add("World");
> 		List subList = uniqueList.subList(1, 2);
> 		subList.add("Hello");
> 		List expectedSubList = Arrays.asList(new Object[] { "World", "Hello" });
> 		List expectedParentList = Arrays.asList(new Object[] { "World", "Hello" });
> 		assertEquals(expectedSubList, subList);
> 		assertEquals(expectedParentList, uniqueList); // fails
> 	}
> 	public void testSubListSetDuplicate() {
> 		List uniqueList = SetUniqueList.decorate(new ArrayList());
> 		uniqueList.add("Hello");
> 		uniqueList.add("World");
> 		List subList = uniqueList.subList(1, 2);
> 		subList.set(0, "Hello");
> 		List expectedSubList = Arrays.asList(new Object[] { "Hello" });
> 		List expectedParentList = Arrays.asList(new Object[] { "Hello" });
> 		assertEquals(expectedSubList, subList);
> 		assertEquals(expectedParentList, uniqueList); // fails
> 	}

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira