snippetjavaMinor
how to write generic extensions for standard collection types in java?
Viewed 0 times
typesgenericextensionscollectionwritestandardjavaforhow
Problem
I am a Java beginner and I am looking for an idiomatic way of writing a function that involves generics. I wrote this helper class (below) that pushes items into a sorted generic collection and I wanted to ask for your feedback. Should I perhaps extends some base class of some collection? Or maybe there is some better approach that follows the Java philosophy?
```
import java.util.Comparator;
import java.util.List;
public abstract class ListExtensions {
public static void addOnCompare(List collection, T item,
Comparator comparator) {
synchronized(collection) {
int i = 0;
int size = collection.size();
if (size == 1) {
int diff = comparator.compare(item, collection.get(0));
switch(diff) {
case 1: i++; break;
default: break;
}
} else {
int range = size - 1;
i = size / 2;
int left = 0;
int right = range;
while(true) {
if (i range) { i = range; break; }
int diff = comparator.compare(item, collection.get(i));
if (diff == 0) break;
else {
if (diff == -1) right = i;
if (diff == 1) left = i;
int near = i + diff;
if (near range) { i = range + 1; break; }
int diff_near = comparator.compare(item, collection.get(near));
if (diff_near == 0) { i = diff_near; break; }
if (diff_near == diff) {
int step = (right-left)/2;
if (step == 0) step = 1;
switch(diff){
case -1:
right = i;
i = i - step; break;
```
import java.util.Comparator;
import java.util.List;
public abstract class ListExtensions {
public static void addOnCompare(List collection, T item,
Comparator comparator) {
synchronized(collection) {
int i = 0;
int size = collection.size();
if (size == 1) {
int diff = comparator.compare(item, collection.get(0));
switch(diff) {
case 1: i++; break;
default: break;
}
} else {
int range = size - 1;
i = size / 2;
int left = 0;
int right = range;
while(true) {
if (i range) { i = range; break; }
int diff = comparator.compare(item, collection.get(i));
if (diff == 0) break;
else {
if (diff == -1) right = i;
if (diff == 1) left = i;
int near = i + diff;
if (near range) { i = range + 1; break; }
int diff_near = comparator.compare(item, collection.get(near));
if (diff_near == 0) { i = diff_near; break; }
if (diff_near == diff) {
int step = (right-left)/2;
if (step == 0) step = 1;
switch(diff){
case -1:
right = i;
i = i - step; break;
Solution
Parts this question have already been answered on StackOverflow - https://stackoverflow.com/a/13529644/139985 So I'm going to treat this as a simple request for a code review.
-
The method name is opaque ...
-
The parameter name
-
The local variable
-
This is convoluted:
Write it as:
-
I don't know what these lines are trying to do, but they are definitely wrong:
The value returned by a comparator has no meaning beyond being -ve, zero, or +ve. So adding it to the index
-
I'm going to hazard a guess that the method is intended to insert
-
Even if this code did work, the performance for a linked list would be poor. The
-
The method name is opaque ...
addOnCompare does not clearly imply a particular action ... and there is no javadoc for the method. This immediately makes code-review hard ... because I have to try and figure out what the code is trying to do before I can comment on whether it is doing that well.-
The parameter name
collection is poor. It is a list, not a collection.-
The local variable
diff_near should be diffNear.-
This is convoluted:
switch(diff) {
case 1: i++; break;
default: break;
}Write it as:
if (diff == 1) { i = 1; }-
I don't know what these lines are trying to do, but they are definitely wrong:
int near = i + diff;
if (near range) { i = range + 1; break; }
int diff_near = comparator.compare(item, collection.get(near));The value returned by a comparator has no meaning beyond being -ve, zero, or +ve. So adding it to the index
i is going to have an unpredictable effect. Indeed, since range never changes, this code makes the behaviour of the entire algorithm too difficult to understand.-
I'm going to hazard a guess that the method is intended to insert
item at the correct position in a previously sorted list. But given the above, I have grave doubts that it will actually work for all possible (valid) comparators and all possible (sorted) input lists.-
Even if this code did work, the performance for a linked list would be poor. The
List.get(int) method for a linked list is O(N), so even an optimal binary search pattern would result in O(Nlog(N)) operations to insert one element into the linked list. A simple traversal of the list followed by an insert would be O(N).Code Snippets
switch(diff) {
case 1: i++; break;
default: break;
}if (diff == 1) { i = 1; }int near = i + diff;
if (near < 0) { i = 0; break; }
if (near > range) { i = range + 1; break; }
int diff_near = comparator.compare(item, collection.get(near));Context
StackExchange Code Review Q#18931, answer score: 3
Revisions (0)
No revisions yet.