2015-08-18 12 views
13

Ich habe einigen hässlichen Code und will es Refactoring:Umgang mit Java Ausnahmen in Konstrukteuren gefangen, mit Endglieder

public class UdpTransport extends AbstractLayer<byte[]> { 
    private final DatagramSocket socket; 
    private final InetAddress address; 
    private final int port; 
    /* boolean dead is provided by superclass */ 

    public UdpTransport(String host, int port) { 
     this.port = port; 
     InetAddress tmp_address = null; 
     try { 
      tmp_address = InetAddress.getByName(host); 
     } catch (UnknownHostException e) { 
      e.printStackTrace(); 
      dead = true; 
      socket = null; 
      address = null; 
      return; 
     } 
     address = tmp_address; 
     DatagramSocket tmp_socket = null; 
     try { 
      tmp_socket = new DatagramSocket(); 
     } catch (SocketException e) { 
      e.printStackTrace(); 
      dead = true; 
      socket = null; 
      return; 
     } 
     socket = tmp_socket; 
    } 
    ... 

Das Problem verursacht die Hässlichkeit die Interaktion zwischen final Mitgliedern und Ausnahmen gefangen. Ich würde gerne die Mitglieder behalten final wenn möglich.

Ich möchte den Code wie folgt bilden, aber der Java-Compiler kann den Kontrollfluss nicht analysieren - es gibt keine Möglichkeit, dass address ein zweites Mal zugewiesen werden konnte, da die erste versuchte Zuweisung ausgelöst haben muss die catch Klausel.

public UdpTransport(String host, int port) { 
    this.port = port; 
    try { 
     address = InetAddress.getByName(host); 
    } catch (UnknownHostException e) { 
     e.printStackTrace(); 
     dead = true; 
     address = null; // can only have reached here if exception was thrown 
     socket = null; 
     return; 
    } 
    ... 

Error:(27, 13) error: variable address might already have been assigned

Jede Beratung?

P.S. Ich habe eine Einschränkung, die die Konstruktoren nicht werfen - sonst wäre das alles einfach.

+0

jemals in Betracht gezogen, dass anstelle der letzten Instanz Variablen verwenden Sie den nicht-final verwenden kann? Wenn Sie sicherstellen möchten, dass die Unveränderlichkeit außerhalb der Klasse liegt, geben Sie eine defensive Kopie in Getter zurück. –

+0

Ist es eine Voraussetzung, dass 'address == null', wenn die Socket-Erstellung fehlgeschlagen ist? – dhke

+0

Warum können Sie nicht eine andere 'tmp_address' haben und am Ende 'address = tmp_address' zuweisen? –

Antwort

13

Lassen Sie den Konstruktor die Ausnahmen auslösen. Das Verlassen von final nicht zugewiesen ist OK, wenn der Konstruktor nicht normal beendet wird, da in diesem Fall kein Objekt zurückgegeben wird.

Der hässlichste Teil Ihres Codes ist das Abfangen von Ausnahmen im Konstruktor und das Zurückgeben der vorhandenen, aber ungültigen Instanz.

+0

Danke, ich werde das ernsthaft darüber nachdenken. – fadedbee

+0

Wenn ich 'neue Layer0 (neue Layer1 (neue Layer2 (params ...), params ...), params ...)' den gesamten Stapel wird fehlschlagen und Layer1 wird nicht wissen, welche Art von Klasse und Konstruktor Parameter Es sollte verwendet werden, um eine neue Layer2 zu erstellen. – fadedbee

+0

Wenn ich mehr darüber nachdenke, ist die Hauptursache, dass ich jeder Schicht in einem Protokollstapel eine Möglichkeit geben muss, ihr Kind zu konstruieren, anstatt ihr ein bereits konstruiertes Kind zu geben. Das ist knifflig ... – fadedbee

4

Zwei Versionen des Konstruktors, in denen Ihre Attribute endgültig bleiben. Beachten Sie, dass ich Ihren ursprünglichen Ansatz nicht als "hässlich" empfinde. Es kann jedoch verbessert werden, da der try-catch-Block für beide Ausnahmen identisch ist. Dies wird mein Konstruktor # 1.

public UdpTransport(String host, int port) { 
    InetAddress byName; 
    DatagramSocket datagramSocket; 

    try { 
     byName = InetAddress.getByName(host); 
     datagramSocket = new DatagramSocket(); 
    } catch (UnknownHostException | SocketException e) { 
     e.printStackTrace(); 
     dead = true; 
     datagramSocket = null; 
     byName = null; 
    } 
    this.port = port; 
    this.socket = datagramSocket; 
    this.address = byName; 
} 

public UdpTransport(int port, String host) { 

    this.port = port; 
    this.socket = createSocket(); 
    this.address = findAddress(host); 
} 

private DatagramSocket createSocket() { 
    DatagramSocket result; 
    try { 
     result = new DatagramSocket(); 
    } catch (SocketException e) { 
     e.printStackTrace(); 
     this.dead = true; 
     result = null; 
    } 
    return result; 
} 

private InetAddress findAddress(String host) { 
    InetAddress result; 
    try { 
     result = InetAddress.getByName(host); 
    } catch (UnknownHostException e) { 
     e.printStackTrace(); 
     this.dead = true; 
     result = null; 
    } 
    return result; 
} 
2

Es gibt keinen gültigen Grund für diesen Konstruktor, Ausnahmen überhaupt abzufangen. Ein Objekt voller null Werte ist für die Anwendung völlig sinnlos. Der Konstruktor sollte diese Ausnahme werfen. Fange es nicht.

8

Wenn Sie private Konstruktoren verwenden können, können Sie Ihren Konstruktor hinter einer öffentlichen statischen Factory-Methode verbergen, die verschiedene Instanzen Ihrer UdpTransport-Klasse zurückgeben kann. Lassen Sie uns sagen:

public final class UdpTransport 
     extends AbstractLayer<byte[]> { 

    private final DatagramSocket socket; 
    private final InetAddress address; 
    private final int port; 
    /* boolean dead is provided by superclass */ 

    private UdpTransport(final boolean dead, final DatagramSocket socket, final InetAddress address, final int port) { 
     super(dead); 
     this.socket = socket; 
     this.address = address; 
     this.port = port; 
    } 

    public static UdpTransport createUdpTransport(final String host, final int port) { 
     try { 
      return new UdpTransport(false, new DatagramSocket(), getByName(host), port); 
     } catch (final SocketException | UnknownHostException ex) { 
      ex.printStackTrace(); 
      return new UdpTransport(true, null, null, port); 
     } 
    } 

} 

Die Lösung über die folgenden Hinweise haben:

  • Es nur einen Konstruktor hat, der die Parameter auf die Felder nur zuweist. So können Sie leicht final Felder haben. Dies ist sehr ähnlich zu dem, was ich erinnere, heißt primären Konstruktoren in C# und wahrscheinlich Scala.
  • Die statische Factory-Methode verbirgt die Komplexität der Instanziierung UdpTransport.
  • Der Einfachheit halber wird die statische Factory-Methode gibt eine Instanz entweder mit beiden socket und address Satz an realen Fällen oder sie auf null ungültigen Zustand anzeigt. Dieser Instanzstatus kann sich leicht von dem Status unterscheiden, der durch den Code in Ihrer Frage erzeugt wird.
  • Mit solchen Factory-Methoden können Sie die tatsächliche Implementierung der zurückgegebenen Instanz ausblenden. Daher ist die folgende Deklaration vollständig gültig: public static AbstractLayer<byte[]> createUdpTransport(final String host, final int port) (beachten Sie den Rückgabetyp). Die Stärke eines solchen Ansatzes besteht darin, dass Sie den zurückgegebenen Wert je nach Bedarf durch eine Unterklasse ersetzen können, wenn dies möglich ist, es sei denn, Sie verwenden UdpTransport - spezifische öffentliche Schnittstelle.
  • Auch wenn Sie mit ungültigen Statusobjekten sind, schätze ich, dann sollte eine ungültige Statusinstanz keinen echten Portwert enthalten, der Folgendes ermöglicht (vorausgesetzt, -1 kann einen ungültigen Portwert angeben oder sogar Nullable Integer if Sie sind frei, die Felder der Klasse und primitive Wrapper zu ändern sind keine Einschränkung für Sie):
private static final AbstractLayer<byte[]> deadUdpTransport = new UdpTransport(true, null, null, -1); 
... 
public static AbstractLayer<byte[]> createUdpTransport(final String host, final int port) { 
    try { 
     return new UdpTransport(false, new DatagramSocket(), getByName(host), port); 
    } catch (final SocketException | UnknownHostException ex) { 
     ex.printStackTrace(); 
     return deadUdpTransport; // it's safe unless UdpTransport is immutable 
    } 
  • Schließlich IMHO, die in solchen Verfahren einen Stack-Trace Druck ist keine gute Idee.
0

Was ist so etwas wie dieses:

public class UdpTransport extends AbstractLayer<byte[]> { 
    private final DatagramSocket socket; 
    private final InetAddress address; 
    private final int port; 

    public static UdpTransport create(String host, int port) { 
     InetAddress address = null; 
     DatagramSocket socket = null; 
     try { 
      address = InetAddress.getByName(host); 
      socket = new DatagramSocket(); 
     } catch (UnknownHostException | SocketException e) { 
      e.printStackTrace(); 
     } 
     return new UdpTransport(socket, address, port); 
    } 

    private UdpTransport(DatagramSocket socket, InetAddress address, int port) { 
     this.port = port; 
     this.address = address; 
     this.socket = socket; 
     if(address == null || socket == null) { 
      dead = true; 
     } 
    } 
    ...