trjhtr

I am trying to compare two Log lines based on some conditions, in order to sort them:

Code:

public static final Comparator<String> HTMLcomparator = new Comparator<String>()
 
 @Override
 public int compare(String line1, String line2)
 
 HTMLLogLine htmlLogLine1 = new HTMLLogLine(line1);
 HTMLLogLine htmlLogLine2 = new HTMLLogLine(line2);
 int fullCompare = 0;

 String requestId1 = htmlLogLine1.getRequestId();
 String requestId2 = htmlLogLine2.getRequestId();

 if(requestId1 != null && requestId2 != null)
 
 fullCompare = requestId1.compareTo(requestId2);
 
 else if(requestId1 == null && requestId2 != null)
 
 fullCompare = -1;
 
 else if(requestId1 != null)
 
 fullCompare = 1;
 
 else
 
 fullCompare = 0;
 

 if(fullCompare == 0)
 
 String security1 = htmlLogLine1.getSecurity();
 String security2 = htmlLogLine2.getSecurity();
 if(security1 != null && security2 != null)
 
 fullCompare = security1.compareTo(security2);
 
 else if(security1 == null && security2 != null)
 
 fullCompare = -1;
 
 else if(security1 != null && security2 == null)
 
 fullCompare = 1;
 
 else
 
 fullCompare = 0;
 
 

 if(fullCompare == 0)
 
 String scenario1 = htmlLogLine1.getScenario();
 String scenario2 = htmlLogLine2.getScenario();

 if(scenario1 != null && scenario2 != null)
 
 fullCompare = scenario1.compareTo(scenario2);

 
 else if(scenario1 == null && scenario2 != null)
 
 fullCompare = -1;
 
 else if(scenario1 != null)
 
 fullCompare = 1;
 
 else
 
 fullCompare = 0;
 
 

 if(fullCompare == 0)
 
 Optional<Instant> timestamp1 = htmlLogLine1.getTimestamp();
 Optional<Instant> timestamp2 = htmlLogLine2.getTimestamp();

 if(timestamp1.isPresent() && timestamp2.isPresent())
 
 fullCompare = timestamp1.get().compareTo(timestamp2.get());
 
 else if(!timestamp1.isPresent() && timestamp2.isPresent())
 
 fullCompare = -1;
 
 else if(timestamp1.isPresent() && !timestamp2.isPresent())
 
 fullCompare = 1;
 
 else
 
 fullCompare = 0;
 

 

 return fullCompare;
 
 ;

How can I reduce the duplicate code and make this compare method concise?

You can reduce the code duplication by moving the logic of comparing two values of which at least one is null to a separate method and call this method like this:

if (requestId1 != null && requestId2 != null) 
 fullCompare = requestId1.compareTo(requestId2);
 else 
 fullCompare = compareWithNull(requestId1, requestId2);


//...

if (security1 != null && security2 != null) 
 fullCompare = security1.compareTo(security2);
 else 
 fullCompare = compareWithNull(security1, security2);


//...

This reduces the code duplication, but you still have duplicate code, namely the procedure of checking whether both values are non-null and proceeding accordingly. So you could also move this logic into a separate method, for instance, into the compare(T, T) method of a separate comparator:

public int compare(T a, T b) 
 if (a == null) 
 return (b == null) ? 0 : -1;
 else if (b == null) 
 return 1;
 else 
 return a.compare(b);
 

The good news is that you don't actually have to do this yourself, because since Java 8, there's a method Comparator.nullsFirst​(Comparator) that does exactly that: It returns a comparator that considers null less than non-null (and equal to null), and if both objects are non-null, compares them using the provided comparator. The code sample above is in fact a slightly modified copy from the source of Comparator.nullsFirst​(Comparator). Since you order non-null elements according to their natural ordering, you would just need to call Comparator.nullsFirst(Comparator.naturalOrder()).

So the first three comparison stages can be reduced to the functionality of these four comparators:

Comparator<String> nullsFirstStringComparator = Comparator.nullsFirst(Comparator.naturalOrder());

Comparator<HTMLLogLine> requestIdComparator = Comparator.comparing(HTMLLogLine::getRequestId, nullsFirstStringComparator);
Comparator<HTMLLogLine> securityComparator = Comparator.comparing(HTMLLogLine::getSecurity, nullsFirstStringComparator);
Comparator<HTMLLogLine> scenarioComparator = Comparator.comparing(HTMLLogLine::getScenario, nullsFirstStringComparator);

Judging by your comment, you already seem to be aware of the method Comparator.thenComparing(Comparator), so there's no need to explain how to chain those comparators using this method.

Unfortunately, the forth comparison stage does not fit into this pattern. Of course, you could ditch the nulls-first comparator and write your own generalized version of it which, instead of comparing the two values with null, checks a given Predicate against the values:

class CustomComparator<T> implements Comparator<T> 
 Predicate<? super T> predicate;
 Comparator<? super T> otherComparator;

 @Override
 public int compare(T o1, T o2) 
 if (predicate.test(o1)) 
 return (predicate.test(o2)) ? 0 : -1;
 else if (predicate.test(o2)) 
 return 1;
 else 
 return otherComparator.compare(o1, o2);
 
 

But I think there would only be a point in doing that if you need it more than once, because otherwise, it would not really be a remedy for code duplication, but just unnecessary code (since you don't really need it for the first three cases due to the pre-existing method Comparator.nullsFirst(Comparator)).

Reducing duplications

As the other answer pointed out,
you could reduce duplication by extracting the comparison logic to a helper method.
I would go a bit further and make the null comparions part of the helper method, for example:

static int compareStrings(String s1, String s2) 
 if (s1 == null && s2 == null) return 0;
 if (s1 == null) return -1;
 if (s2 == null) return 1;
 return s1.compareTo(s2);

Then, using early returns,
the compare method could become much more compact:

@Override
public int compare(String line1, String line2) 
 HTMLLogLine htmlLogLine1 = new HTMLLogLine(line1);
 HTMLLogLine htmlLogLine2 = new HTMLLogLine(line2);
 int cmp;

 String requestId1 = htmlLogLine1.getRequestId();
 String requestId2 = htmlLogLine2.getRequestId();
 cmp = compareStrings(requestId1, requestId2);

 if (cmp != 0) return cmp;

 String security1 = htmlLogLine1.getSecurity();
 String security2 = htmlLogLine2.getSecurity();
 cmp = compareStrings(security1, security2);

 if (cmp != 0) return cmp;

 String scenario1 = htmlLogLine1.getScenario();
 String scenario2 = htmlLogLine2.getScenario();
 cmp = compareStrings(scenario1, scenario2);

 if (cmp != 0) return cmp;

 Optional<Instant> timestamp1 = htmlLogLine1.getTimestamp();
 Optional<Instant> timestamp2 = htmlLogLine2.getTimestamp();
 return compareOptionalInstants(timestamp1, timestamp2);

Repeated object creation

How do you plan to use this comparator?
Keep in mind that the compare method will create two HTMLLogLine objects every time it's called.
If you will use this for only a few strings,
or if you know that compare will not be called on the same strings repeatedly,
then everything's fine.

On the other hand, if you have a potentially large list of strings,
and you will use this comparator as a parameter to List::sort or Collections::sort, then there will be some unnecessary object creation.
The sorting process compares different pairs of values,
and even with smart divide-and-conquer algorithms,
some strings will inevitably appear in multiple pairs,
and so multiple HTMLLogLine instances will get created for the same strings, which is wasteful.

To avoid unnecessary object creation,
you could do the following:

1. Create a list of HTMLLogLine from the list of strings


2. Create a list of indexes (values 0 to number of values)


3. Sort the list of indexes by applying the comparator to their corresponding values


4. Replace the list of strings by mapping the sorted indexes to the original list

This way precisely one HTMLLogLine instance will be created for each original string.
The drawback of this technique is that it uses more memory overall.

The implementation can be quite compact.
Reusing the nullsFirstStringComparator from the other answer,
and given the original list of strings in strings,
it could go something like this:

List<HTMLLogLine> list2 = strings.stream().map(HTMLLogLine::new).collect(Collectors.toList());

List<String> sorted = IntStream.range(0, strings.size()).boxed().sorted(
 Comparator.<Integer, String>comparing(i -> list2.get(i).getRequestId(), nullsFirstStringComparator)
 .thenComparing(i -> list2.get(i).getSecurity(), nullsFirstStringComparator)
 .thenComparing(i -> list2.get(i).getScenario(), nullsFirstStringComparator)
 .thenComparing(i -> list2.get(i).getTimestamp(), emptyFirstTimestampComparator)
).map(strings::get).collect(Collectors.toList());

The implementation of emptyFirstTimestampComparator is straightforward, if a bit tedious:

Comparator<Optional<Instant>> emptyFirstTimestampComparator = (a, b) -> 
 if (!a.isPresent() && !b.isPresent()) return 0;
 if (!a.isPresent()) return -1;
 if (!b.isPresent()) return 1;
 return a.get().compareTo(b.get());
;

Note that a more compact form is also possible using lambdas,
but I don't recommend this,
because I think it's harder to read:

Comparator<Optional<Instant>> emptyFirstTimestampComparator = (a, b) -> a.map(instant1 -> b.map(instant1::compareTo).orElse(1)).orElseGet(() -> !b.isPresent() ? 0 : -1);