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?

  • 5
    Not an answer but just a note: Don't ever throw exceptions from property getters. – Brandon Aug 24 '15 at 14:26
  • 3
    To elaborate on Brandon's point: stackoverflow.com/a/1488488/569777 – MetaFight Aug 24 '15 at 17:09
  • 1
    Sure: The general idea (msdn.microsoft.com/en-us/library/…, stackoverflow.com/questions/1488472/…, among others) is that properties should do as little as possible, be lightweight and should represent the clean, public facing state of your object, with indexers being a slight exception. It's also unexpected behavior to see exceptions in a watch window for a property during debugging. – Brandon Aug 24 '15 at 17:24
  • 1
    You shouldn't (need to) write code that throws in a property getter. That doesn't mean you are required to hide and swallow any exceptions which occur due to an object getting into an unsupported state. – Ben Aug 24 '15 at 19:17
  • 4
    Why can't you just do Room.Client.Id? Why do you want to wrap that into Room.ClientId? If its to check for a client then why not something like Room.HasClient { get { return client == null; } } that is more straight forward and still let people do the normal Room.Client.Id when they actually need the id? – James Aug 24 '15 at 21:59
up vote 25 down vote accepted

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.

  • 2
    Nice. I like the thought "To keep things simple I wouldn't allow the Client property of any Room instance to ever be null. " That is indeed better than to question what to do when it is null – Michel Aug 24 '15 at 13:40
  • @Michel if you later decide to change that requirement, you can replace the null value with a NullObject (or rather NullClient) which would then still work with your existing code and allow you to define behaviour for a non existent client if need be. The question is if the "no client" case is part of the business logic or not. – null Aug 24 '15 at 13:58
  • 3
    Totally correct. Don't write a single character of code to support an unsupported state. Just let it throw a NullRef. – Ben Aug 24 '15 at 19:16
  • @Ben Disagree; MS guidelines explicitly state that property getters should not throw exceptions. And allowing null refs to be thrown when a more meaningful exception could be thrown instead is just lazy. – Andy Aug 25 '15 at 12:57
  • 1
    @Andy understanding the reason for the guideline will allow you to know when it doesn't apply. – Ben Aug 26 '15 at 22:36

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".

  • 5
    +1 for " I don't see e.g. why the information that the ClientId is a long has to be carved into stone into the signature of Room" – Michel Aug 24 '15 at 14:13
  • Your English is fine. ;) Just from reading this answer, I would not have known that your native tongue is not English without you saying so. – jpmc26 Aug 24 '15 at 21:21
  • Points B & C are good; RE: a, its a convenience method, and the fact that room has a reference to Client might just be an implementation detail (it could be argued that Client should not be publicly visible) – Andy Aug 25 '15 at 12:59

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

  • 1
    Shouldn't these be ArgumentNullExceptions? – Mathieu Guindon Aug 25 '15 at 2:36
  • 4
    No. Program code should never throw a NullReferenceException, it is thrown by the .Net VM "when there is an attempt to dereference a null object reference". You should be throwing an ArgumentNullException, which is thrown "when a null reference (Nothing in Visual Basic) is passed to a method that does not accept it as a valid argument." – Pharap Aug 25 '15 at 3:19
  • 2
    @Pharap I'm a Java programmer attempting to write C# code, I figured I'd make mistakes like that. – durron597 Aug 25 '15 at 3:21
  • @durron597 I should have guessed that from the use of the term 'final' (in this case the corresponding C# keyword is readonly). I would have just edited but I felt a comment would serve to better explain why (for future readers). If you hadn't already given this answer I would be giving the exact same solution. Immutability is also a good idea, but it's not always as easy as one hopes. – Pharap Aug 25 '15 at 3:33
  • 2
    Properties generally shouldn't thrown exceptions; instead of a setter that throws ArgumentNullException, provide a SetClient method instead. Someone posted the link to an answer regarding this on the question. – Andy Aug 25 '15 at 13:01

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... */
}
  • 1
    I think "you'll end up duplicating everything a client already offers" needs to be in bold or at least italics. – Pharap Aug 25 '15 at 3:36

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.

  • That NotSupportedException is being misused, you should be using an ArgumentNullException instead. – Pharap Aug 25 '15 at 3:38

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.

  • but the type of clientid is a nullable long then? – Michel Aug 25 '15 at 10:13
  • @Michel well, if a room must have a Client, put a null check in the ctor. Then var clientId = room.Client.Id will be both safe and long. – Jodrell Aug 25 '15 at 11:28

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.