Verificare nulă în implementarea .equals (Revizuirea codului, Java)

Nenad Bulatovic a intrebat.

Am moștenit un cod moștenit pe care acum încerc să îl curăț.

Am impresia că a 2-a null verificare este redundantă, a se vedea comentariile:

private java.lang.Object __equalsCalc = null;
    public synchronized boolean equals(java.lang.Object obj) {
        if (!(obj instanceof ArrayOfColumn)) return false; // Here is first check if the obj is not null, if there is instaceof ArrayOfColumn then it's not null
        ArrayOfColumn other = (ArrayOfColumn) obj;
        if (obj == null) return false; // Therefore this check is redundant and "return false" will never execute, so it's dead code
        if (this == obj) return true;
        if (__equalsCalc != null) {
            return (__equalsCalc == obj);
        }
        __equalsCalc = obj;
        boolean _equals;
        _equals = true && 
            ((this.column==null && other.getColumn()==null) || 
             (this.column!=null &&
              java.util.Arrays.equals(this.column, other.getColumn())));
        __equalsCalc = null;
        return _equals;
    }

Altceva?

Comentarii

  • ce este __equalsCalc ce caută acolo?-  > Por Ratchet Freak.
  • Poți să adaugi un context despre ce încerci să obții?-  > Por RubberDuck.
  • Postul este discutat pe meta-  > Por rolfl.
  • @ratchetfreak Tocmai am găsit ce face de fapt __equalsCalc come: stackoverflow.com/questions/367905/… Ești de acord cu acest răspuns?-  > Por Nenad Bulatovic.
1 răspunsuri
rolfl

În primul rând, da, aveți dreptate că a doua verificare null este redundantă. Dacă obj este nul, atunci metoda va returna false la prima verificare:

    if (!(obj instanceof ArrayOfColumn)) return false;

Bineînțeles, ar fi mai bine să fie scris:

    if (!(obj instanceof ArrayOfColumn)) {
        return false;
    }

Dar, aceasta este cea mai mică dintre preocupările mele. Probabil că metoda sincronizată equals este acolo pentru că alte metode sunt sincronizate, dar sincronizarea are un cost. Dacă nu sunteți sigur că aveți nevoie de ea, eliminați-o.

În plus, metodele sincronizate sunt rareori cea mai bună soluție. În mod normal, este mai bine să aveți un control mai strict al încuietorilor, astfel încât nimeni altcineva în afară de dvs. să le poată deține. Acest lucru înseamnă, în mod normal, utilizarea unui „monitor” privat, intern, pentru sincronizare:

private final Objct lock = new Object();

private boolean equals(Object obj) {
    synchronized(lock) {
        .....
    }
}

Utilizarea blocării private împiedică alte persoane să vă blocheze aplicația prin utilizarea clasei dvs. ca monitor propriu. Imaginați-vă că clasa dvs. se numește MyClass, , iar cineva o face:

 private MyClass instance = new MyClass();

 synchronized(instance) {
     Thread.sleep(10000);
 }

Dacă aceștia ar face ceea ce s-a întâmplat mai sus, metodele dvs. din alte fire de execuție nu s-ar executa niciodată, iar aplicația dvs. s-ar „bloca”.

Acum, în ceea ce privește __equalsCalc variabilă. Aceasta nu face nimic util. Este setată și apoi ștearsă în interiorul metodei sincronizate și, ca urmare, nu va avea niciodată, niciodată, nicio valoare utilă. Este complet redundantă și o pierdere de timp.

Oh, și ce este cu calificarea completă java.lang.Object? Folosiți pur și simplu Object.

Eu aș rescrie metoda sub forma (folosind aceeași metodă sincronizată pentru a fi compatibilă cu alte metode din clasă):

public synchronized boolean equals(Object obj) {
    if (!(obj instanceof ArrayOfColumn)) {
        return false;
    }
    if (obj == this) {
        return true;
    }
    ArrayOfColumn other = (ArrayOfColumn) obj;
    if (getColumn() == null) {
        return other.getColumn() == null;
    }
    return other.getColumn() != null && Arrays.equals(column, other.getColumn());
}

Rețineți că aveți în continuare o vulnerabilitate de sincronizare – coloana celeilalte coloane ar putea fi schimbată între momentul în care se verifică dacă coloana sa este nulă și cel în care se folosește coloana în metoda „equals”. Ați putea obține în continuare excepții de pointer nul dacă cineva modifică cealaltă coloană în mijlocul procedurii de egalitate. Ar trebui să luați în considerare sincronizarea și pentru cealaltă coloană:

public synchronized boolean equals(Object obj) {
    if (!(obj instanceof ArrayOfColumn)) {
        return false;
    }
    if (obj == this) {
        return true;
    }
    ArrayOfColumn other = (ArrayOfColumn) obj;
    synchronized(other) {
        if (getColumn() == null) {
            return other.getColumn() == null;
        }
        return other.getColumn() != null && Arrays.equals(column, other.getColumn());
    }
}

Din păcate, sincronizarea încrucișată de acest fel poate duce la blocaje dacă nu sunteți atent.

Comentarii

  • Tocmai am aflat ce face de fapt __equalsCalc vine: stackoverflow.com/questions/367905/… Sunteți de acord cu acest răspuns?-  > Por Nenad Bulatovic.
  • Sunt foarte sigur că o metodă sincronizată equals nu are nici un sens. Ați făcut o analiză bună, menționând problemele de „vulnerabilitate” și „deadlock”, deoarece acestea ar fi preocupările mele pentru a evita o astfel de implementare. Și cred că o astfel de sincronizare încrucișată ar putea fi utilă la nivel local, dar nu și în orice context GLOBAL.-  > Por oopexpert.

Tags: