2
\$\begingroup\$

I have the class SeguimientoSeccion with this relevant attributes

public class SeguimientoSeccion implements Serializable {
    private Integer id;

    private Transportista transportista;
    private String fijacion;
    private Integer month;
    private Integer year;
}

I created a custom comparator to sort by

  1. year - Integer
  2. month - Integer
  3. fijacion - String
  4. Transportista.nombre - String

SeguimientoSeccionComparator.java (class I want to improve)

public class SeguimientoSeccionComparator implements Comparator<SeguimientoSeccion> 
{
    public int compare(SeguimientoSeccion o1, SeguimientoSeccion o2) 
    {
        int result = o1.getYear().compareTo(o2.getYear());

        if (result == 0) {
            result = o1.getMonth().compareTo(o2.getMonth());
        }

        if (result != 0) return result;

        if (o1.getFijacion() == null && o2.getFijacion() == null)
            result = 0;
        else if (null == o1.getFijacion())
            result = 1;
        else if (null == o2.getFijacion())
            result = -1;
        else
            result = o1.getFijacion().compareTo(o2.getFijacion());

        if (result != 0) return result;

        if (o1.getFijacion() == null && o2.getFijacion() == null)
            result = 0;
        else if (null == o1.getTransportista())
            result = 1;
        else if (null == o2.getTransportista())
            result = -1;
        else
            result = o1.getTransportista().getNombre().compareTo(o2.getTransportista().getNombre());

        return result;
    }
}

EXTRA INFO:

equals and hashCode autogenerated by Eclipse IDE:

@Override
public int hashCode() {
    final int prime = 31;
    int result = 1;
    result = prime * result
            + ((fijacion == null) ? 0 : fijacion.hashCode());
    result = prime * result + ((id == null) ? 0 : id.hashCode());
    result = prime * result + ((month == null) ? 0 : month.hashCode());
    result = prime * result
            + ((transportista == null) ? 0 : transportista.hashCode());
    result = prime * result + ((year == null) ? 0 : year.hashCode());
    return result;
}
@Override
public boolean equals(Object obj) {
    if (this == obj)
        return true;
    if (obj == null)
        return false;
    if (getClass() != obj.getClass())
        return false;
    SeguimientoSeccion other = (SeguimientoSeccion) obj;
    if (fijacion == null) {
        if (other.fijacion != null)
            return false;
    } else if (!fijacion.equals(other.fijacion))
        return false;
    if (id == null) {
        if (other.id != null)
            return false;
    } else if (!id.equals(other.id))
        return false;
    if (month == null) {
        if (other.month != null)
            return false;
    } else if (!month.equals(other.month))
        return false;
    if (transportista == null) {
        if (other.transportista != null)
            return false;
    } else if (!transportista.equals(other.transportista))
        return false;
    if (year == null) {
        if (other.year != null)
            return false;
    } else if (!year.equals(other.year))
        return false;
    return true;
}
\$\endgroup\$
  • \$\begingroup\$ Are you on Java 8 ? \$\endgroup\$ – Spotted Nov 13 '15 at 11:04
  • \$\begingroup\$ @Spotted unfortunatelly, nope, JDK1.7.0.51x64 \$\endgroup\$ – Jordi Castilla Nov 13 '15 at 12:43
  • 2
    \$\begingroup\$ And.... my answer is 54 seconds late... \$\endgroup\$ – rolfl Nov 13 '15 at 12:45
  • 1
    \$\begingroup\$ (There was a C&P-error in the intended "both null"-condition before the getTransportista() invocations. The getNombre().compareTo() goes unchecked…) \$\endgroup\$ – greybeard Jan 13 '16 at 0:13
2
\$\begingroup\$

You should consider using the native language/library support for work like this.

Let me prime that, by adding this private function to your clas:

private Integer getTransNombre() {
    return transportista == null ? null : transportista.getNombre();
}

That gets the number, if the transportista is not null. Now, using that function, we can create a reusable comparator instance using the tricks that Java 8 introduces. Consider these chained comparators:

private static final Comparator<SeguimientoSeccion> COMP = Comparator.comparing(SeguimientoSeccion::getYear)
        .thenComparing(SeguimientoSeccion::getMonth)
        .thenComparing(SeguimientoSeccion::getFijacion, Comparator.nullsLast(Comparator.naturalOrder()))
        .thenComparing(SeguimientoSeccion::getTransNombre, Comparator.nullsLast(Comparator.naturalOrder()));

How does that work? It gets details about the instances, if they compare as "equals" then the next test is run. You can sort nulls to the end, and you can go from there.

If you still need your comparator class, it could be:

public class SeguimientoSeccionComparator implements Comparator<SeguimientoSeccion> {
    public int compare(SeguimientoSeccion o1, SeguimientoSeccion o2) {
        return COMP.compare(o1, o2);
    }
}
| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

Given

/** Note: this class has a natural ordering
 *        that is inconsistent with equals. */
static class Transportista
    implements Comparable<Transportista> {
    String nombre;
    String getNombre() { return nombre; }
    @Override
    public int compareTo(Transportista o) { return 0; }
}
/** mock Java 8 nullsLast comparison */
static int nullsLast(Comparable c, Comparable x) {
    if (c == x)
        return 0;
    if (null == c)
        return 1;
    if (null == x)
        return -1;
    return c.compareTo(x); 
}

, compare() could look

/** by {@code year, month, fijacion, Transportista.nombre} */
@Override
public int compare(SeguimientoSeccion o1, SeguimientoSeccion o2) 
{
    int result;
    final Transportista t1, t2;
    if (   0 != (result = o1.getYear().compareTo(o2.getYear()))
        || 0 != (result = o1.getMonth().compareTo(o2.getMonth()))
        || 0 != (result = nullsLast(o1.getFijacion(),
                                    o2.getFijacion()))
        || 0 != (result = nullsLast(t1 = o1.getTransportista(),
                                    t2 = o2.getTransportista())))
        return result;
    return nullsLast(t1.getNombre(), t2.getNombre());
}

(null handling of nombres differs from the question ever so slightly.)

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.