57

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?

AgostinoX
  • 841
  • 1
  • 6
  • 12
  • 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 02 '14 at 16:05
  • @Rotem return `null`. – scriptin Nov 02 '14 at 16:12
  • 2
    @scriptin Then following that logic, `displayCompleteNameOnInvoice` can just throw an exception if `getCompleteName` returns `null`, can't it? – Rotem Nov 02 '14 at 16:19
  • 2
    @Rotem it can. The question is about if it should. – scriptin Nov 02 '14 at 16:23
  • @Rotem it is the more logical behaviour that doesn't require additional explanation.say you have an email class, with getRecipient() and a send() methods.If you don't set the recipient and call the send() method it's *obvious* that you will get an error.But if you call getRecipient()? Not only it's not obvious, but if you return an error you didn't allow the class user to simply check if the field is set or not, forcing them to introduce error-check logic just to workaround the problem.returning null would be correct for a simple field, but here the Whole getter semantic seems to be misleading – AgostinoX Nov 02 '14 at 16:24
  • @AgostinoX I don't follow your very last sentence "but here the whole getter semantic seems to be misleading". Why is that? A getter should get a value if one exists, or null if one does not, is that not so? – Rotem Nov 02 '14 at 16:29
  • @Rotem, normally a empty string is better then null, as it can be shown in a UI without getting errors. – Ian Nov 02 '14 at 19:11
  • 22
    On the subject, [Falsehoods Programmers Believe About Names](http://www.kalzumeus.com/2010/06/17/falsehoods-programmers-believe-about-names/) is a very good read on why 'first name' and 'surname' are very narrow concepts. – Lars Viklund Nov 03 '14 at 07:26
  • 2
    Just a general note: never throw exceptions for something you can control. If it's to print a value, initialize it as an empty string; if you need a value, don't allow the object to be created without it (like DeadMG says). Exceptions are meant for faults that you cannot control (a file not beeing in the right location, a network connection that is down,... ). If a user needs to enter text, don't let him submit the form without the necessary input (and try to validate it before letting him submit; e-mail requiring '@' and '.[ext]', phone fields only containing numbers,...). – Andreas Nov 03 '14 at 09:13
  • 1
    @scriptin returning null [should generally be avoided (pg 110 (141 in file))](https://www.ufm.edu/images/0/04/Clean_Code.pdf) – Holloway Nov 03 '14 at 09:46
  • @Andreas An empty string is not the same thing as the absence of a string. If you want to return the absence of something, then [return the absence of something](http://docs.oracle.com/javase/8/docs/api/java/util/Optional.html). That aside, "don't throw exceptions for something you can control" is also too dogmatic. It's reasonable to use an exception in cases where you *are* in control, but failure occurs rarely (because exceptions are expensive in Java), and is generally handled several calls up the stack (because inspecting results for failure in every intermediate layer is silly.) – Doval Nov 04 '14 at 17:17
  • @Doval An empty string doesn't trigger NPE's. I personally prefer an empty array/list/... or 0-value primitive since I find it to be a lot cleaner (iterating over an empty array simply does nothing). Optional is only available in Java 8 and not everyone has the luxury of always being able to use the latest version. I haven't encountered a situation where I (or another developer) was in full control and an exception was necessary. In those cases it tends to be called a bug (or 'feature'). Admittedly, I've only been working with java for 2-3 years (nearly fresh from school). – Andreas Nov 05 '14 at 09:31
  • 9
    If your object can have an invalid state, you've already screwed up. – user253751 Nov 05 '14 at 10:08
  • @Andreas It's not hard to write Optional from scratch. I don't have the luxury of Java 8 either. – Doval Nov 05 '14 at 12:16
  • The business object(s) bound to a UI where the user can enter data pretty much have to be made to accept invalid data; for example a first name required on an order. you shouldn't be able to save the order perhaps, but until the user enters basic information its invalid. – Andy Nov 06 '14 at 02:25
  • @Andy There's a difference between the input to a form and an order proper. A user gives you a public bundle of data with no hidden structure, no behavior, and no guarantees; it's not a "business object", it's the equivalent of a C struct. You then take that and attempt to produce an order, which does have invariants and whose implementation might be hidden. If an order is produced at all, it should never be in an invalid state. – Doval Nov 06 '14 at 13:01
  • @Doval I'm talking about OO, where data and behavior are combined into one class. You're talking procedural or functional code. In OO, there's no reason the order can't be invalid. Depending on the use case, you might be able to save it (allowing the user to finish it later, even with missing data) but not place it (putting the order into a state to be fulfilled where the shopper can't change it anymore, and which does require it to be valid). The Order objects entire job is allow a user to create and place an order; when the order is started it pretty much won't be in a valid state. – Andy Nov 06 '14 at 13:53
  • @Doval In a good OO design, the user is working with the order object, they aren't just typing data into a vacuum. – Andy Nov 06 '14 at 13:53
  • @Andy I'm not talking about functional or procedural code. I'm saying you don't create an order object until after you've validated the user's data, which is very much not an object. To use a different analogy, a string is not an abstract syntax tree. A parser takes a string, which may or may not be a program, and generates an abstract syntax tree from it only if it is. There's a difference between an object and the data it's created from. – Doval Nov 06 '14 at 14:39
  • @Doval You're talking about data as if its separate from behavior, and in OO its not, it never is. If you force a separate as you're describing, you're BACK to procedural code. Your C struct example proves my point; in C there are data structures which are separate from behaviors (functions). The user puts data into the structure and it gets passed around. Its the anemic domain model antipattern, and you end up not even doing OO. In OO data a user enters into the order is just as much part of the order as is the fact that the order requires a name to transition it into a Placed state. – Andy Nov 06 '14 at 15:08
  • @Andy Except I never said anything about passing the struct around. I said it gets validated and an object gets created from that. The *object* is then passed around. You're setting up straw men. – Doval Nov 06 '14 at 15:17
  • @Doval So what behavior does your order have then? How is the data structure valid (does it valid itself)? – Andy Nov 06 '14 at 15:29

6 Answers6

151

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

DeadMG
  • 36,794
  • 8
  • 70
  • 139
  • 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 02 '14 at 16:00
  • Agostino no DeadMG is right. – RibaldEddie Nov 02 '14 at 16:38
  • 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 02 '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 02 '14 at 17:09
  • 7
    +1 this is the simplest correct answer. @AgostinoX one big advantage of this approach is that the exception is thrown when the Object is created, and the calling code might be able to do something about it, like present a UI dialog. By the time you are printing mailing labels it is way too late to correct the mistake. – user949300 Nov 02 '14 at 17:12
  • 5
    @user949300 without an explanation, this answer may become useless in case if someone else posts an opposite opinion. For example, if someone posts a claim like _"It won't help if you don't permit your class to be constructed without assigning a name."_, how would this answer help reader to pick of two opposing opinions? – gnat Nov 02 '14 at 17:16
  • 6
    @gnat First I'd reply that using three negatives in a sentence is confusing. :-) Though I guess I agree with you and DeadMG could elaborate slightly on his answer. For example, his two comments could be added to the answer. – user949300 Nov 02 '14 at 17:22
  • 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 02 '14 at 17:46
  • 6
    @gnat: If someone posted that hypothetical alternate answer, I would help readers choose between them by downvoting it, since it is incorrect. – Michael Shaw Nov 02 '14 at 19:21
  • 3
    It's the "how do I fit these square wheels so that my car drives smoothly?" problem. Square wheels are sometimes a must, like it or not, but here the tone of OP's question indicates they're keeping their mind open to different design suggestions and aren't hellbent on sticking to the original idea. – Konrad Morawski Nov 02 '14 at 19:24
  • 2
    @gnat Oh, hop off it. This is a perfectly fine answer. It says exactly what is wrong, and how to fix it. – alternative Nov 02 '14 at 23:26
  • 2
    @AgostinoX: classes that only permit certain methods to be called in certain states should be avoided, but if they cannot, then those methods should throw "IllegalStateException". It doesn't matter if those methods are "getters" or not. – kevin cline Nov 02 '14 at 23:47
  • 2
    "Do not permit your class to be constructed without assigning a name.": Good solution. Additionally, one should make the member variables `final` (or `const` in C++ speak). – Giorgio Nov 03 '14 at 08:50
  • 1
    @AgostinoX using immutable objects is really not that radical, but a standard recommended practice at least with java. Mutable objects should be considered a radical exception, as they suffer from a lot of potential issues related to thread safety, for example. Of course, standard javabeans are against that policy, but they're mostly frowned upon nowadays. – eis Nov 03 '14 at 12:56
  • 1
    This answer only works for fields that can't validly be optional. An Invoice might allow for a PurchaseOrder to be assigned, and yet not required. Or perhaps you don't need names at all, and just assigning a CompanyName is sufficient. In any such (common) case, the OP is left with the idea that either the field is made mandatory or...well, nothing. – BrianH Nov 03 '14 at 15:13
  • 9
    If they can be validly optional, there's no reason for him to throw in the first place. – DeadMG Nov 03 '14 at 23:08
  • 1
    As has been said: If the attribute is optional, then the state of the object is not invalid, obviating the need for an exception. If the attribute is mandatory, then the object should not be created without the attribute being valid. I.E., throw the exception at instantiation. – CuriousRabbit Nov 07 '14 at 18:43
  • 1
    If you are using an ORM then you need to provide a zero arg constructor, and if your DB has an error then you could run into this scenario. – user90766 Mar 11 '15 at 02:46
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).

Basil Bourque
  • 1,000
  • 5
  • 10
back2dos
  • 29,980
  • 3
  • 73
  • 114
  • 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 03 '14 at 08:25
  • 1
    @KonradMorawski: I am confused. Are you clarifying or contradicting my statement? ;) – back2dos Nov 03 '14 at 09:20
  • 3
    I tried to complement it... – Konrad Morawski Nov 03 '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.

Community
  • 1
  • 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 02 '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. –  Nov 02 '14 at 22:02
  • 1
    ... the place to do that guarantee is during the construction of the object - either with a constructor, Builder or Factory. In any case, the key is to make certain that you are handling issues of what the state of the object is early on rather than later when someone is consuming the data with certain expectations of what the object is and is not. –  Nov 02 '14 at 22:04
  • OK but there are classes that do many things, without even violating single responsibility principle because that things are interconnected and refactoring to multiple classes would by more pain than gain.The builder pattern is a solution of course, but quite expensive for simple cases,IMHO.Think to a ORM class.How many times we save a record that still is incomplete?An invoice without recipient, because the user has to be able to come back and complete it after a while?In all those cases validating constructors and builder pattern seem to be solutions,so to say, abastract. – AgostinoX Nov 02 '14 at 22:17
  • 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. –  Nov 02 '14 at 22:24
  • 2
    A related blog post to read: [Designing Object Initialization](http://www.artima.com/designtechniques/desinitP.html) 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. –  Nov 02 '14 at 22:34
  • Thanks for the link, seems interesting and very in topic.The other thing that i started to realize from your previous post and become clear here is that while I use exceptions 'freely' to guide development (this is the kind of exception that you discover at design time),on the other hand when you see an exception,you say:in this code there is an invalid state that should be prevented.But my idea here was to have a 'handy' way of doing things;name and surname are 'simple' data. In a class with many fields,we usually don't want to create an entire builder just to ensure that one of them is valid – AgostinoX Nov 02 '14 at 22:47
  • 2
    @AgostinoX with a class that has many fields, you start wondering if there are *too many* fields and if it should be refactored into multiple classes. The design philosophy that I draw from for constructor design is that the class must *always* be in a valid state - it may not be *complete*, but it is *valid* and further setters can continue to modify that (though always keeping it valid). In this way, I don't need to worry about null checks in most places because it can't happen and thus becomes easier to reason about and use. –  Nov 03 '14 at 01:32
  • @AgostinoX as an aside, since this is getting quite long here, you should visit us in [chat] (The Whiteboard), I am sure those of us in there would enjoy such a discussion. The best time would likely be the end of your work day (its the start of ours) or evening. Chat tends to be most active between 9-5 M-F for [north american hours](http://www.timebie.com/timezone/centralrome.php) (though there are some europeans that do have a schedule that would closely match yours). I would love to go more into the design options than comments allow for. –  Nov 03 '14 at 03:05
  • 1
    @AgostinoX: "Think to a ORM class. How many times we save a record that still is incomplete?" Maybe that's the issue here. A record and an object are two different things. A record is just compound data. An object is behavior (on top of **hidden** state). The two couldn't be any more different. A record is interfaced with by reading and writing its components. An object should be interfaced according to "Tell, don't ask" and should sensibly respond to whatever you tell it. It should only throw exceptions if the caller told it *exceptional* non-sense. Full stop. – back2dos Nov 03 '14 at 08:09
  • 6
    "Do the thing that is easiest to reason about. " That. **That** – Ben Nov 04 '14 at 14:19
  • 1
    @MichaelT - well said in the answer and many comments! I loathe that "beans" have led many people to pepper everything with getters and setters; I prefer to think in Constructors and Mutators. It should be _impossible_ to put an object in an invalid state. If you have get/set every field you might as well program using a **C** `stuct` which can have an indeterminate state at any time. – Stephen P Nov 06 '14 at 01:42
  • 1
    @StephenP beans are ideally these rather 'dumb' objects that are passed between layers - and as that, they work. Unfortunately, many people want smart OO objects (that was part of the reason we're writing OO, right?) and that is where the two ideas (lets add a bean nature to this smart object or lets add smarts to this bean) get into conflict and lead to *other* messes. There is nothing fundamentally wrong with passing a list of beans to a jsp. Putting getters and setters that aren't dumb or are exposing too much of the internals of an object - thats a problem. –  Nov 06 '14 at 01:54
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....
}
Rotem
  • 1,946
  • 16
  • 16
  • 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 02 '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 02 '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 02 '14 at 19:23
  • @DavidWallace: Which is the situation described in the OP. So every case that he's discussing. – DeadMG Nov 02 '14 at 19:36
  • 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 02 '14 at 19:40
  • @DavidWallace: No that is not "what we should code". It is pretty much the opposite of the null object pattern. – back2dos Nov 03 '14 at 07:55
  • 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 03 '14 at 11:14
5

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.

Community
  • 1
user541686
  • 8,074
  • 8
  • 38
  • 49
  • Comments are not for extended discussion; this conversation has been [moved to chat](http://chat.stackexchange.com/rooms/18469/discussion-on-answer-by-mehrdad-should-a-getter-throw-an-exception-if-its-object). – maple_shaft Nov 07 '14 at 01:00
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).

sampathsris
  • 565
  • 5
  • 17