HiveBrain v1.2.0
Get Started
← Back to all entries
snippetjavaMinor

how to write generic extensions for standard collection types in java?

Submitted by: @import:stackexchange-codereview··
0
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;

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 ... 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.