Handling Java exceptions caught in constructors, with final members

Question

I have some ugly code and want to refactor it:

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

The issue causing the ugliness is the interaction between final members and caught exceptions. I would like to keep the members final if possible.

I would like to form the code as follows, but the Java compiler cannot analyse the control flow - there is no way that address could be assigned a second time, as the first attempted assignment must have thrown for control to have reached the catch clause.

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

Any advice?

P.S. I have a constraint which is that the constructors do not throw - otherwise this would all be easy.

Answer 1

Let the constructor to throw the exceptions. Leaving final unassigned is OK if the constructor does not terminate normally, as in this case no object is returned.

The most ugly part of your code is catching exceptions in constructor and then returning the existing, but invalid instance.

Answer 2

If you're free to use private constructors, you can hide your constructor behind a public static factory method, that can return different instances of your UdpTransport class. Let's say:

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

}

The solution above may have the following notes:

• It has only one constructor that only assigns the parameters to the fields. Thus you can easily have final fields. This is very similar to what I remember is called primary constructors in C# and probably Scala.
• The static factory method hides the complexity of UdpTransport instantiation.
• For simplicity, the static factory method returns an instance with either both socket and address set to real instances, or them set to null indicating invalid state. So this instance state may slightly differ from the state produced by the code in your question.
• Such factory methods allow you to hide the real implementation of the instance they return, thus the following declaration is fully valid: public static AbstractLayer<byte[]> createUdpTransport(final String host, final int port) (note the return type). The power of such approach is that you can substitute the returned value by any subclass depending on your needs if it's possible unless you use UdpTransport-specific public interface.
• Also if you're fine with invalid state objects, I guess, then an invalid state instance should not hold a real port value allowing you to make the following (assuming -1 can indicate an invalid port value, or even nullable Integer if you're free to change the fields of your class and primitive wrappers are not a restriction for you):

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
    }

• Finally, IMHO, printing a stack trace in such methods is not a good idea.

Answer 3

Two versions of the constructor, where your attributes stay final. Note, that I don't consider your original approach as generally 'ugly'. It can be improved though, as the try-catch-block is identical for both exceptions. This becomes my constructor #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;
}

Answer 4

There is no valid reason for this constructor to catch exceptions at all. An object full of null values is of no use whatsoever to the application. The constructor should throw that exception. Not catch it.

Answer 5

What about something like this:

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