How to implement a property on class A which refers to a property of a child object of class A

Question

We have this code which, when simplified, looks like this:

public class Room
{
    public Client Client { get; set; }

    public long ClientId
    {
        get
        {
            return Client == null ? 0 : Client.Id;
        }
    }
}

public class Client 
{
    public long Id { get; set; }
}

Now we have three viewpoints.

1) This is good code because the Client property should always be set (i.e. not null) so the Client == null will never occur and the Id value 0 denotes a false Id anyway (this is the opinion of the writer of the code ;-))

2) You can not rely on the caller to know that 0 is a false value for Id and when the Client property should always be set you should throw an exception in the get when the Client property happens to be null

3) When the Client property should always be set you just return Client.Id and let the code throw a NullRef exception when the Client property happens to be null.

Which of these is most correct? Or is there a fourth possibility?

Answer 1

It smells like you should limit the number of states your Room class can be in.

The very fact that you're asking about what to do when Client is null is a hint that Room's state space is too large.

To keep things simple I wouldn't allow the Client property of any Room instance to ever be null. That means the code within Room can safely assume the Client is never null.

If for some reason in the future Client becomes null resist the urge to support that state. Doing so will increase your maintenance complexity.

Instead, allow the code to fail and fail fast. After all, this is not a supported state. If the application gets itself into this state you've already crossed a line of no return. The only reasonable thing to do then is to close the application.

This might happen (quite naturally) as the result of an unhandled null reference exception.

Answer 2

Just a few considerations:

a) Why is there a getter specifically for the ClientId when there's already a public getter for the Client instance itself? I don't see why the information that the ClientId is a long has to be carved in stone in the signature of Room.

b) Regarding the second opinion you could introduce a constant Invalid_Client_Id.

c) Regarding opinion one and three (and being my main point): A room must always have a client? Maybe it's just semantics but this doesn't sound right. Maybe it would be more appropriate to have completely separate classes for Room and Client and another class that ties them together. Maybe Appointment, Reservation, Occupancy? (This depends on what you're actually doing.) And on that class you can enforce the constraints "must have a room" and "must have a client".

Answer 3

I disagree with all three of the opinions. If Client can never be null, then don't even make it possible for it to be null!

1. Set the value of Client in the constructor
2. Throw an ArgumentNullException in the constructor.

So your code would be something like:

public class Room
{
    private Client theClient;

    public Room(Client client) {
        if(client == null) throw new ArgumentNullException();
        this.theClient = client;
    }

    public Client Client { 
        get { return theClient; }
        set 
        {
            if (value == null) 
                throw new ArgumentNullException();
            theClient = value;
        }
    }

    public long ClientId
    {
        get
        {
            return theClient.Id;
        }
    }
}

public class Client 
{
    public long Id { get; set; }
}

---

This is unrelated to your code, but you would probably be better off using an immutable version of Room by making theClient readonly, and then if the client changes, making a new room. This will improve the thread safety of your code in addition to the null safety of the other aspects of my answer. See this discussion on mutable vs immutable

Answer 4

The second and third options should be avoided - the getter should not smack the caller with an exception they have no control over.

You should decide whether Client can ever be null. If so, you should provide a way for a caller to check whether it is null before accessing it (e.g., bool ClientIsNull property).

If you decide that Client can never be null, then make it a required parameter to the constructor and throw the exception there if a null is passed in.

Finally, the first option also contains a code smell. You should let Client deal with its own ID property. It seems like overkill to code a getter in a container class that simply calls a getter on the contained class. Just expose the Client as a property (otherwise, you'll end up duplicating everything a Client already offers).

long clientId = room.Client.Id;

If Client can be null, then you'll at least give responsibility to the caller:

if (room.Client != null){
    long clientId = room.Client.Id;
    /* other code follows... */
}

Answer 5

If a null Client property is a supported state, consider using a NullObject.

But most likely this is an exceptional state, so you should make it impossible (or just not very convenient) to end up with a null Client:

public Client Client { get; private set; }

public Room(Client client)
{
    Client = client;
}

public void SetClient(Client client)
{
    if (client == null) throw new ArgumentNullException();
    Client = client;
}

If it is not supported however, do not waste time on half baked solutions. In this case:

return Client == null ? 0 : Client.Id;

You've 'solved' the NullReferenceException here but at a great cost! This would force all callers of Room.ClientId to check for 0:

var aRoom = new Room(aClient);
var clientId = aRoom.ClientId;
if (clientId == 0) RestartRoombookingWizard();
else ContinueRoombooking(clientid);

Strange bugs can arize with other devs (or yourself!) forgetting to check the "ErrorCode" return value at some point in time.
Play it safe en fail fast. Allow the NullReferenceException to be thrown and "gracefully" catch it somewhere higher up the call stack instead of allowing the caller to mess things up even more...

On a different note, if you are so keen to expose the Client and also the ClientId think about TellDontAsk. If you ask too much from objects instead of telling them what you want to achieve, you may end coupling everything together, making change more difficult later on.

Answer 6

As of c# 6.0, which is now released, you should just do this,

public class Room
{
    public Room(Client client)
    {
        this.Client = client;
    }

    public Client Client { get; }
}

public class Client
{
    public Client(long id)
    {
        this.Id = id;
    }

    public long Id { get; }
}

Raising a property of Client to Room is an obvious breach of encapsulation and the DRY principle.

If you need to access the Id of a Client of a Room you can do,

var clientId = room.Client?.Id;

Note the use of the Null Conditional Operator, clientId will be an long?, if Client is null, clientId will be null, otherwise clientId will have the value of Client.Id.