14

私はいくつかの醜いコードを持っていて、それをリファクタリングしたい:

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

醜さの原因となる問題は、finalメンバー間の相互作用とキャッチされた例外です。finalできればメンバーを維持したい。

次のようにコードを作成したいと思いますが、Java コンパイラは制御フローを分析できません。制御が節addressに到達するには、最初に試行された割り当てがスローされたに違いないため、2 回目の割り当てを行う方法はありません。catch

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

何かアドバイス？

PSコンストラクターがスローしないという制約があります-そうでなければ、これはすべて簡単です。

4

5 に答える 5

14

コンストラクターに例外をスローさせます。コンストラクfinalターが正常に終了しない場合は、オブジェクトが返されないため、割り当てられていないままでも問題ありません。

コードの最も醜い部分は、コンストラクターで例外をキャッチし、既存の無効なインスタンスを返すことです。

于 2015-08-18T09:59:15.410 に答える
8

プライベート コンストラクターを自由に使用できる場合は、クラスのさまざまなインスタンスを返すパブリック静的ファクトリ メソッドの背後にコンストラクターを隠すことができますUdpTransport。まあ言ってみれば：

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);
        }
    }

}

上記の解決策には、次の注意事項がある場合があります。

  • パラメータをフィールドに割り当てるだけのコンストラクタが 1 つだけあります。したがって、フィールドを簡単に作成できfinalます。これは、C# やおそらく Scala でプライマリ コンストラクターと呼ばれるものと非常によく似ています。
  • UdpTransport静的ファクトリ メソッドは、インスタンス化の複雑さを隠します。
  • 簡単にするために、静的ファクトリ メソッドは、 と の両方が実際のインスタンスに設定されたインスタンス、または無効な状態を示すように設定されたインスタンスをsocket返しaddressますnull。したがって、このインスタンスの状態は、質問のコードによって生成される状態とはわずかに異なる場合があります。
  • このようなファクトリ メソッドを使用すると、それらが返すインスタンスの実際の実装を隠すことができるため、次の宣言は完全に有効です: public static AbstractLayer<byte[]> createUdpTransport(final String host, final int port)(戻り値の型に注意してください)。UdpTransportこのようなアプローチの利点は、特定のパブリック インターフェイスを使用しない限り、可能であれば、必要に応じて任意のサブクラスで戻り値を置き換えることができることです。
  • また、無効な状態オブジェクトで問題がなければ、無効な状態インスタンスは実際のポート値を保持してはならず、次のことを行うことができます (-1無効なポート値を示すことができると仮定するか、Integer自由であればnullable にすることもできます)。クラスのフィールドを変更し、プリミティブ ラッパーは制限ではありません):
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
    }
  • 最後に、IMHO、そのようなメソッドでスタック トレースを出力することはお勧めできません。
于 2015-08-18T10:43:28.183 に答える
4

コンストラクターの 2 つのバージョン。属性は最終的なままです。あなたの元のアプローチを一般的に「醜い」とは考えていないことに注意してください。ただし、try-catch-block は両方の例外で同一であるため、改善することができます。これが私のコンストラクター #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;
}
于 2015-08-18T09:56:26.613 に答える
2

このコンストラクターが例外をキャッチする正当な理由はまったくありません。値でいっぱいのオブジェクトnullは、アプリケーションにとってまったく役に立ちません。コンストラクターはその例外をスローする必要があります。それをキャッチしないでください。

于 2015-08-18T10:09:59.507 に答える