Should a getter throw an exception if its object has invalid state?

Question

I often run into this problem, especially in Java, even if I think it's a general OOP issue. That is: raising an exception reveals a design problem.

Suppose that I have a class that has a String name field and a String surname field.

Then it uses those fields to compose the complete name of a person in order to display it on some sort of document, say an invoice.

public void String name;
public void String surname;

public String getCompleteName() {return name + " " + surname;}

public void displayCompleteNameOnInvoice() {
    String completeName = getCompleteName();
    //do something with it....
}

Now I want to strengthen the behavior of my class by throwing an error if the displayCompleteNameOnInvoice is called before the name has been assigned. It seems a good idea, doesn't it?

I can add a exception-raising code to getCompleteName method. But in this way I'm violating an 'implicit' contract with the class user; in general getters aren't supposed to throw exceptions if their values aren't set. Ok, this is not a standard getter since it does not return a single field, but from the user point of view the distinction may be too subtle to think about it.

Or I can throw the exception from inside the displayCompleteNameOnInvoice. But to do so I should test directly name or surname fields and doing so I would violate the abstraction represented by getCompleteName. It's this method responsibility to check and create the complete name. It could even decide, basing the decision on other data, that in some cases it is sufficient the surname.

So the only possibility seems to change the semantic of the method getCompleteName to composeCompleteName, that suggests a more 'active' behavior and, with it, the ability of throwing an exception.

Is this the better design solution? I'm always looking for the best balance between simplicity and correctness. Is there a design reference for this issue?

Answer 1

Do not permit your class to be constructed without assigning a name.

Answer 2

The answer provided by DeadMG pretty much nails it, but let me phrase it a bit differently: Avoid having objects with invalid state. An object should be "whole" in the context of the task it fulfills. Or it should not exist.

So if you want fluidity, use something like the Builder Pattern. Or anything else that is a separate reification of an object in construction as opposed to the "real thing", where the latter one is guaranteed to have valid state and is the one that actually exposes the operations defined on that state (e.g. getCompleteName).

Answer 3

Don't have an object with an invalid state.

The purpose of a constructor or builder is to set the state of the object to something that is consistent and usable. Without this guarantee, problems crop up with improperly initialized state in objects. This issue becomes compounded if you're dealing with concurrency and something can access the object before you are completely done setting it up.

So, the question for this part is "why are you allowing the name or surname to be null?" If that isn't something that is valid in the class for it to work properly, don't allow it. Have a constructor or builder that properly validates the creation of the object and if it isn't right, raise the issue at that point. You may also wish use one of the @NotNull annotations that exists to help communicate that "this cannot be null" and enforce it in coding and analysis.

With this guarantee in place, it becomes much easier to reason about the code, what it does, and not have to throw exceptions in odd places or put excessive checks around getter functions.

Getters that do more.

There is quite a bit out there on this subject. You've got How Much Logic in Getters and What should be allowed inside getters and setters? from here and getters and setters performing additional logic over on Stack Overflow. This is an issue that comes up again and again in class design.

The core of this comes from:

in general getters aren't supposed to throw exceptions if their values aren't set

And you are right. They shouldn't. They should look 'dumb' to the rest of the world and do what is expected. Putting too much logic in there leads to issues where the law of least astonishment is violated. And lets face it, you really don't want to be wrapping the getters with a try catch because we know how much we love doing that in general.

There are also situations where you must use a getFoo method such as the JavaBeans framework and when you have something from EL calling expecting to get a bean (so that <% bar.foo %> actually calls getFoo() in the class - setting aside the 'should the bean be doing the composition or should that be left to the view?' because one can easily come up with situations where one or the other can clearly be the right answer)

Realize also that it is possible for a given getter to be specified in an interface or to have been part of the previously exposed public API for the class that is getting refactored (a previous version just had 'completeName' that was returned and a refactoring broke it into two fields).

At the end of the day...

Do the thing that is easiest to reason about. You will spend more time maintaining this code than you will spend designing it (though the less time you spend designing, the more time you will spend maintaining). The ideal choice is to design it in such a way that it won't take as much time to maintain, but don't sit there thinking about it for days either - unless this really is a design choice that will have days of implications later.

Trying to stick with semantic purity of the getter being private T foo; T getFoo() { return foo; } will get you into trouble at some point. Sometimes the code just doesn't fit that model and the contortions that one goes through to try to keep it that way just doesn't make sense... and ultimately makes it harder to design.

Accept sometimes that the ideals of the design can't be realized the way you want them in the code. Guidelines are guidelines - not shackles.

Answer 4

It seems like no one is answering your question.
Unlike what people like to believe, "avoid invalid objects" is not often a practical solution.

The answer is:

Yes it should.

Easy example: FileStream.Length in C#/.NET, which throws NotSupportedException if the file is not seekable.

Answer 5

I'm not a Java guy, but this seems to adhere to both constraints you presented.

getCompleteName does not throw an exception if the names are uninitialized, and displayCompleteNameOnInvoice does.

public String getCompleteName()
{
    if (name == null || surname == null)
    {
        return null;
    }
    return name + " " + surname;
}

public void displayCompleteNameOnInvoice() 
{
    String completeName = getCompleteName();
    if (completeName == null)
    {
        //throw an exception.
    }
    //do something with it....
}

Answer 6

You have the whole checking name thing upside down.

getCompleteName() does not have to know which functions will be utilizing it. Thus, not having a name when calling displayCompleteNameOnInvoice() is not getCompleteName()s responsibility.

But, name is a clear prerequisite for displayCompleteNameOnInvoice(). Thus, it should take the responsibility of checking the state (Or it should delegate the responsibility of checking to another method, if you prefer).