Vojtech Szocs has posted comments on this change. Change subject: webadmin,userportal: Consolidate SortedListModel vs. SearchableListModel ......................................................................
Patch Set 1: (11 comments) http://gerrit.ovirt.org/#/c/31101/1/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SearchableListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SearchableListModel.java: Line 837: lastSelectedItems.add(item); Line 838: } Line 839: } Line 840: Line 841: if (useComparator(value)) { > This means: "perform sorting if there's something to sort by". Patch http://gerrit.ovirt.org/#/c/28392/ changed SortedListModel.sortItems method in the following way: - SortedSet<T> sortedItems = new TreeSet<T>(comparator); - sortedItems.addAll(items); - return sortedItems; + List<T> sortedList = new ArrayList<T>(items); + Collections.sort(sortedList, comparator); + return sortedList; As a result, TreeSet (implementation of SortedSet) is no longer used when applying comparator on model's items. In consequence, condition: value instanceof SortedSet is not relevant anymore. This is why I removed above condition. In current SortedListModel.sortItems implementation, we are returning a new (sorted) ArrayList containing model's items. There is no association between (sorted) ArrayList and the comparator being used, therefore condition: "if the collection isn't already sorted by that ordering" cannot be determined. In other words, condition: value instanceof SortedSet is not relevant anymore (as I wrote above). If you have objections or believe I missed something, please elaborate. Line 842: Collection<T> sortedItems = sortItems(value); Line 843: itemsChanging(sortedItems, items); Line 844: items = sortedItems; Line 845: } else { http://gerrit.ovirt.org/#/c/31101/1/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModel.java: Line 82: // Sort items after updating comparator Line 83: Collection<T> items = getItems(); Line 84: if (items != null) { Line 85: Collection<T> maybeSortedItems = useComparator(items) ? sortItems(items) : new ArrayList<T>(items); Line 86: super.setItems(maybeSortedItems); > Instead of the two lines above: > Can't you just call setItems(items) here, as what you are doing here is > essentially the same. UiCommon models love to override parent's methods without calling appropriate super method. This leads to inconsistent & copy-paste-like code that is hard to maintain. This is the general problem of UiCommon code. In this particular case, I did not simply call setItems(items) here because: * SearchableListModel overrides setItems(items) _without_ calling super method * if I called setItems(items) here, it would ultimately delegate to overridden setItems(items) in SearchableListModel (which doesn't call super method), so setItems(items) in this class would not be called at all > The only difference I see is the is the false case where you return new > ArrayList<T>(items) instead of just items. Not entirely sure why you do this > though. Wrapping items in new ArrayList is to ensure that condition in SearchableListModel.setItems(items): if (items != value) is passed in order to update model's items. For example, in a situation when comparator has been set and now is nullified, we wrap items in new ArrayList to ensure that model's items are really updated as they should be. Line 87: } Line 88: } Line 89: Line 90: @Override Line 82: // Sort items after updating comparator Line 83: Collection<T> items = getItems(); Line 84: if (items != null) { Line 85: Collection<T> maybeSortedItems = useComparator(items) ? sortItems(items) : new ArrayList<T>(items); Line 86: super.setItems(maybeSortedItems); > I agree. Plus if this is a SearchableListModel, you'll want the overridden > Plus if this is a SearchableListModel, you'll want the overridden method to > be triggered rather than that of ListModel. This is a valid point. However, as I wrote above, the general problem is bad code - method override doesn't call super method, it re-implements any logic provided by super method. This leads to inconsistency and situations like this one. I can call setItems(items) here, but it will mean that when using SortedListModel directly (no subclass/override), items which are already sorted in setComparator will be sorted _again_ in setItems. On the other hand, I fully agree that overidden setItems should be called rather than super setItems. So I'll change this to become: setItems(maybeSortedItems); Line 87: } Line 88: } Line 89: Line 90: @Override Line 88: } Line 89: Line 90: @Override Line 91: public void setItems(Collection<T> items) { Line 92: Collection<T> maybeSortedItems = useComparator(items) ? sortItems(items) : items; > I think this should be called without the useComparator() condition. Maybe Can you be more specific? What's wrong with useComparator() check? My understanding: * useComparator is a method (encapsulating any conditions that determine whether items should be sorted or not) - we can unit test it * items _can_ be null * comparator _can_ be null Maybe the name of the method could be changed from "useComparator" to "applySorting" or similar, though. Line 93: super.setItems(maybeSortedItems); Line 94: } Line 95: Line 96: protected boolean useComparator(Collection<T> items) { Line 92: Collection<T> maybeSortedItems = useComparator(items) ? sortItems(items) : items; Line 93: super.setItems(maybeSortedItems); Line 94: } Line 95: Line 96: protected boolean useComparator(Collection<T> items) { > Considering comments here and in SearchableListModel, I think this method i I disagree, please see my comment above. Line 97: return items != null && comparator != null; Line 98: } Line 99: Line 100: protected Collection<T> sortItems(Collection<T> items) { Line 97: return items != null && comparator != null; Line 98: } Line 99: Line 100: protected Collection<T> sortItems(Collection<T> items) { Line 101: if (comparator == null) { > I don't think this is an erroneous case - if no comparator is supplied, usu Yes, when comparator is null, Collections.sort will rely on natural ordering of objects. However, in order for natural ordering of objects to work, each object must implement Comparable interface. I don't think this is the case (in general) for business entities used in UI code. I don't think that we should rely on natural ordering of objects here. Natural ordering is already reflected by server providing items in a specific order (assuming no SORTBY or equivalent param is defined). Item sorting is based solely on the use of a comparator here. Line 102: throw new IllegalStateException("comparator cannot be null"); //$NON-NLS-1$ Line 103: } Line 104: Line 105: List<T> sortedList = new ArrayList<T>(items); http://gerrit.ovirt.org/#/c/31101/1/frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModelTest.java File frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModelTest.java: Line 93: }; Line 94: } Line 95: Line 96: @Test Line 97: public void testUseComparator_nullItems() { > I don't really care about conventions, but this underscore thing won't be l AFAIK, this is a common convention used when writing Java unit tests: @Test public void testMethodName_description() { ... } The goal here is to make method name human-readable (using underscores) and meaningful. I'm using "test" prefix to indicate that this is a specific test case. This prefix could be omitted as well, but I see no harm in it. References: * http://osherove.com/blog/2005/4/3/naming-standards-for-unit-tests.html * http://stackoverflow.com/a/1594049 * http://stackoverflow.com/a/2861186 Test code needs special conventions because a method name acts as a description of a particular test case. People should adapt conventions to match the code. People shouldn't blindly follow the same conventions for all code. Line 98: SortedListModel<TestItem> testedModel = new SortedListModel<TestItem>(makeValueComparator()); Line 99: assertThat(testedModel.useComparator(null), is(false)); Line 100: } Line 101: Line 117: testedModel.sortItems(null); Line 118: } Line 119: Line 120: @Test(expected = IllegalStateException.class) Line 121: public void testSortItems_nullComparator() { > As per my comment in SortedListModel, I think this is a valid case that sho Please see my response to that comment, I don't think that relying on natural ordering of objects is a good idea. Line 122: SortedListModel<TestItem> testedModel = new SortedListModel<TestItem>(); Line 123: testedModel.sortItems(SOME_ITEMS); Line 124: } Line 125: Line 123: testedModel.sortItems(SOME_ITEMS); Line 124: } Line 125: Line 126: @Test Line 127: public void testSortItems_noItems() { > Common functionality between sortItems() test methods (the assertions, basi In general, you are right, but I tend to avoid over-complicating the test code. When someone sees existing "test boilerplate" code, he must first learn/understand it in order to use it. When someone sees plain-old test methods, he immediately sees what's tested and how. The goal here is to keep unit tests simple and readable. As I mentioned above, people should adapt conventions to match the code. Line 128: SortedListModel<TestItem> testedModel = new SortedListModel<TestItem>(makeValueComparator()); Line 129: Collection<TestItem> initial = Collections.emptyList(); Line 130: Collection<TestItem> sorted = testedModel.sortItems(initial); Line 131: assertNotSame(sorted, initial); Line 168: SortedListModel<TestItem> testedModel = new SortedListModel<TestItem>(); Line 169: Collection<TestItem> initial = Arrays.asList(ITEM_2_1, ITEM_2_2, ITEM_1_2, ITEM_1_1); Line 170: testedModel.setItems(initial); Line 171: Collection<TestItem> sorted = testedModel.getItems(); Line 172: assertSame(sorted, initial); > Not sure this is what I would expect. My expectation would depend on whethe > My expectation would depend on whether the class implements Comparable or not. Current patch avoids any reliance on natural ordering of objects. Also, TestItem doesn't implement Comparable interface. Sorting is done solely via explicit comparator. > Otherwise, I would expect it to either keep the same ordering or be sorted by > hashCode() or whatever Java does by default, either would be fine. Items are either sorted (via comparator) or not sorted (retaining their original order). I don't understand what you mean by "sorted by hashCode() or whatever Java does by default". > But I don't think either of these will currently happen - won't you throw an > exception for the null comparator? Model can be created with or without comparator. SortedListModel<TestItem> testedModel = new SortedListModel<TestItem>(); creates the model without comparator. The expectation is that when no comparator is set, no sorting will occur, so items you set to model are the same as items retrieved via getItems() method. I don't understand why would we throw exception for null comparator. Null comparator is completely valid, it means that no sorting will occur on items. Line 173: } Line 174: Line 175: @Test Line 176: public void testSetItems_noItems() { Line 207: public void testSetComparator_nullToValueComparator() { Line 208: SortedListModel<TestItem> testedModel = new SortedListModel<TestItem>(); Line 209: Collection<TestItem> initial = Arrays.asList(ITEM_2_1, ITEM_2_2, ITEM_1_2, ITEM_1_1); Line 210: Collection<TestItem> expected = Arrays.asList(ITEM_1_2, ITEM_1_1, ITEM_2_1, ITEM_2_2); Line 211: testedModel.setItems(initial); > Here I would plug another line to test the ordering, and then again after t As I wrote above, if a model doesn't have comparator, no sorting (i.e. no re-order) will occur. SortedListModel<TestItem> testedModel = new SortedListModel<TestItem>(); testedModel.setItems(initial); // no comparator, no sorting Collection<TestItem> actual = testedModel.getItems(); // initial and actual are the same Line 212: testedModel.setComparator(makeValueComparator()); Line 213: Collection<TestItem> sorted = testedModel.getItems(); Line 214: assertNotSame(sorted, initial); Line 215: assertThat(sorted.isEmpty(), is(false)); -- To view, visit http://gerrit.ovirt.org/31101 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id10b84c2fb476ed2240c33d72f6432335650df33 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
