55

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?

  • 8
    "in general getters aren't supposed to throw exceptions if their values aren't set." - What are they supposed to do then? – Rotem Nov 2 '14 at 16:05
  • 2
    @scriptin Then following that logic, displayCompleteNameOnInvoice can just throw an exception if getCompleteName returns null, can't it? – Rotem Nov 2 '14 at 16:19
  • 2
    @Rotem it can. The question is about if it should. – scriptin Nov 2 '14 at 16:23
  • 22
    On the subject, Falsehoods Programmers Believe About Names is a very good read on why 'first name' and 'surname' are very narrow concepts. – Lars Viklund Nov 3 '14 at 7:26
  • 9
    If your object can have an invalid state, you've already screwed up. – immibis Nov 5 '14 at 10:08
151

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

  • 9
    It's interesting but quite a radical solution. Many times your classes have to be more fluid, allowing for states that become a problem only if and when certain methods are called otherwise you'll end up with a proliferaiton of small classes that require too much explanation to be used. – AgostinoX Nov 2 '14 at 16:00
  • 71
    This is not a comment. If he applies the advice in the answer, he will no longer have the problem described. It is an answer. – DeadMG Nov 2 '14 at 17:09
  • 14
    @AgostinoX: You should only make your classes fluid if absolutely necessary. Otherwise, they should be as invariant as possible. – DeadMG Nov 2 '14 at 17:09
  • 25
    @gnat: you make a great work here questioning the quality of every question and every answer on PSE, but sometimes you are IMHO a little bit too nitty. – Doc Brown Nov 2 '14 at 17:46
  • 9
    If they can be validly optional, there's no reason for him to throw in the first place. – DeadMG Nov 3 '14 at 23:08
75

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

  • 7
    So if you want fluidity, use something like the builder pattern - fluidity, or immutability (unless that's what you mean somehow). If one wants to create immutable objects, but need to defer setting some values, then the pattern to follow is to set them one by one on a builder object and only create an immutable one once we've gathered all the values required for the correct state... – Konrad Morawski Nov 3 '14 at 8:25
  • 1
    @KonradMorawski: I am confused. Are you clarifying or contradicting my statement? ;) – back2dos Nov 3 '14 at 9:20
  • 3
    I tried to complement it... – Konrad Morawski Nov 3 '14 at 10:05
40

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.

  • 2
    Obviously my example was just an excuse to improve the coding style by reasoning on it, not a blocking issue. But I'm quite perplexed by the importance that you and others seem to give to constructors.In theory they keep their promise. In practice, they becomes cumbersome to mantain with inheritance, not usable when you extract an interface from a class, not-adopted by javabeans and other frameworks like spring if i'm not going wrong. Under the umbrella of a 'class' idea live many different categories of objects. A value object may well have a validating constructor, but this is just one type. – AgostinoX Nov 2 '14 at 21:55
  • 2
    Being able to reason about code is key to being able to write code using an API, or being able to maintain the code. If a class has a constructor that allows it to be in an invalid state it makes it much harder to think through "well, what does this do?" because now you have to consider more situations than the designer of the class considered or intended. This extra conceptual load comes with the penalty of more bugs and a longer time to write code. On the other hand, if you can look at the code and say "name and surname will never be null", it becomes much easier to write code using them. – user40980 Nov 2 '14 at 22:02
  • 2
    Incomplete does not necessarily mean invalid. An invoice without a receipt can be in progress, and the public API that it presents would reflect that such is a valid state. If surname or name being null is a valid state for the object, then handle it accordingly. But if it's not a valid state - causing methods within the class to throw exceptions, it should be prevented being in that state from the point it was initialized. You can pass around incomplete objects, but passing around ones that are invalid is problematic. If a null surname is exceptional, try to prevent it earlier than later. – user40980 Nov 2 '14 at 22:24
  • 2
    A related blog post to read: Designing Object Initialization and note the four approaches for initializing an instance variable. One of them is to allow it to be in an invalid state, though note that it throws an exception. If you are trying to avoid this, that approach isn't a valid one and you will need to work with the other approaches - which involve ensuring a valid object at all times. From the blog: "Objects that can have invalid states are harder to use and harder to understand than those that are always valid." and that is key. – user40980 Nov 2 '14 at 22:34
  • 5
    "Do the thing that is easiest to reason about. " That. That – Ben Nov 4 '14 at 14:19
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....
}
  • To expand why this is a correct solution: This way the caller of getCompleteName() doesn't need to care if the property is actually stored or if it is computed on the fly. – Bart van Ingen Schenau Nov 2 '14 at 16:59
  • 18
    To expand on why this solution is batshit insane, you have to check for nullity on every single call to getCompleteName, which is hideous. – DeadMG Nov 2 '14 at 18:40
  • No, @DeadMG, you only have to check it in cases where an exception should be thrown if getCompleteName returns null. Which is the way it should be. This solution is correct. – Dawood ibn Kareem Nov 2 '14 at 19:23
  • 1
    @DeadMG Yes, but we don't know what OTHER uses there are of getCompleteName() in the rest of the class, or even in other parts of the program. In some cases, returning null may be exactly what is required. But while there is ONE method whose spec is "throw an exception in this case", that's EXACTLY what we should code. – Dawood ibn Kareem Nov 2 '14 at 19:40
  • 4
    There may be situations where this is the best solution, but I cannot imagine any. For most cases, this seems overly complicated: One method throws with incomplete data, another returns null. I'm afraid this is likely to be used incorrectly by callers. – sleske Nov 3 '14 at 11:14
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.

1

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

protected by gnat Nov 4 '14 at 14:29

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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