Check property for null upon every use?

Question

I just have a quick question to see what the best practice would be when making your own class.

Let's say this class has one private member which gets initialized in the constructor, would I have to then check to see if this private member is null in another public, non-static method? Or is it save to assume that the variable will not be null and therefore not have to add that check?

For example, like the following, is the check for null absolutely necessary.

// Provides Client connections.
public TcpClient tcpSocket;

/// <summary>
/// Creates a telnet connection to the host and port provided.
/// </summary>
/// <param name="Hostname">The host to connect to. Generally, Localhost to connect to the Network API on the server itself.</param>
/// <param name="Port">Generally 23, for Telnet Connections.</param>
public TelnetConnection(string Hostname, int Port)
{
        tcpSocket = new TcpClient(Hostname, Port);
}

/// <summary>
/// Closes the socket and disposes of the TcpClient.
/// </summary>
public void CloseSocket()
{
    if (tcpSocket != null)
    {
        tcpSocket.Close();
    }  
}

So, I have made some changes based on all your answers, and I'm wondering if maybe this will work better:

private readonly TcpClient tcpSocket;

public TcpClient TcpSocket
{
    get { return tcpSocket; }
}

int TimeOutMs = 100;

/// <summary>
/// Creates a telnet connection to the host and port provided.
/// </summary>
/// <param name="Hostname">The host to connect to. Generally, Localhost to connect to the Network API on the server itself.</param>
/// <param name="Port">TODO Generally 23, for Telnet Connections.</param>
public TelnetConnection(string Hostname, int Port)
{
        tcpSocket = new TcpClient(Hostname, Port);
}

/// <summary>
/// Closes the socket and disposes of the TcpClient.
/// </summary>
public void CloseSocket()
{
    if (tcpSocket != null)
    {
        tcpSocket.Close();
    }  
}

Thanks.

In your code sample tcpSocket is NOT private. And what would stop CloseSocket from being call more than once? — paparazzo, Aug 23 '12 at 14:13

score 6 · Accepted Answer · answered Aug 23 '12 at 14:14

You've made the property public, so any code using this class can set the reference to null, causing any operation on it to throw a NullReferenceException.

If you want the user of your class to live with that (which is defendable): no, you don't have to check for null.

You could also make the property like public TcpClient tcpSocket { get; private set; }, so external code can't set it to null. If you don't set the tcpSocket to null inside your class, it will never be null since the constructor will always be called.

Thank you for the response, would you be able to look at my edit above to see if this would work better? Thanks — Feytality, Aug 23 '12 at 14:40

paparazzo · Answer 2 · 2012-08-23T15:17:01.473

I don't understand why you open a connection in the ctor then have public method to close it. If you are creating a connection in the ctor then that typically means it is a connection you want for the life of the class.

If you are asking how to make sure the connection is closed when the class is disposed then implement IDisposable.

IDisposable Interface

Since it is private it should not be null but you should check for connected.

   if (tcpSocket.Connected)
   {
       tcpSocket.Close();
   }

score 1 · Answer 3 · answered Aug 23 '12 at 14:20

Generally, it is safe if you can make sure a field is not null. You could call it a class invariant. In your code, however, tcpSocket is not private, so anyone can set its value to null.

I'd recommend you to make the field a property with a private setter (unless you can make it completely private). This makes you sure that no external (i.e. uncontrollable!) code modifies the reference. This in turn makes you able to guarantee tcpSocket is not null.

Check property for null upon every use?

3 Answers3