This looks more like tryGetStuff() to me...
–
Bill MichellDec 23 '11 at 9:46

8

This is not a 'getter'. This term is used for the read accessor of a property, not a method you accidentally put a 'get' in the name.
–
Boris YankovDec 27 '11 at 20:00

3

I don't know if that second example is a fair example of this clean code book you mention, or someone getting the wrong end of the stick about it, but one thing that brittle mess is not, is clean code.
–
Jon HannaJan 24 '14 at 17:36

16 Answers
16

As a rule of thumb, the way I do things is as follows: if getting the stuff is computationally cheap, (or if most chances are that it will be found in the cache,) then your style of getStuff() is fine. If getting the stuff is known to be computationally expensive, so expensive that advertising its expensiveness is necessary at the interface, then I would not call it getStuff(), I would call it calculateStuff() or something like that, so as to indicate that there will be some work to do.

In either case, the way work tells you to do things is lame, because getStuff() will blow up if loadStuff() has not been called in advance, so they essentially want you to complicate your interface by introducing order-of-operations complexity to it. Order-of-operations is pretty much about the worst kind of complexity that I can think of.

+1 for mentioning the order-of-operations complexity. As a workaround, maybe work will ask me to always call loadStuff() in the constructor, but that would be bad too because it means it will always have to be loaded. In the first example, data is lazily loaded only when needed, which is as good as it can be.
–
this.lau_Dec 23 '11 at 9:49

4

I usually follow the rule of "if it's really cheap, use a property getter. If it is expensive, use a function". That usually serves me well, and naming appropriately like you indicated to emphasize it seems also good to me.
–
Denis TrollerDec 23 '11 at 15:37

3

if it can fail - it's not a getter. In this case what if the DB link is down?
–
Martin BeckettDec 23 '11 at 15:58

3

+1, I'm a bit in shock at how many wrong answers have been posted. Getters/Setters exist to hide implementation details, otherwise the variable should just be made public.
–
IzkataDec 23 '11 at 17:20

2

Don't forget that requiring the loadStuff() function be called prior to the getStuff() function also means that the class is not properly abstracting away what is going on under the hood.
–
rjziiJan 4 '12 at 15:24

But getting data from a database is a whole lot more than "logic". It involves a series of very expensive operations where lots of things can go wrong, and in a non-deterministic way. I'd hesitate do that implicitly in a getter.

On the other hand, most ORMs support lazy loading of collections, which is basically exactly what you're doing.

They tell me there should be as little logic as possible in getters and setters.

There needs to be as much logic as is necessary to fulfil the needs of the class. My personal preference is for as little as possible, but when maintaining code, you usually have to leave the original interface with the existing getters/setters, but put lots of logic in them to correct newer business logic (as an example, a "customers" getter in a post-911 environment has to meet "know your customer" and OFAC regulations, combined with a company policy prohibiting the appearance of customers from certain countries from appearing [such as Cuba or Iran]).

In your example, I prefer yours and dislike the "uncle bob" sample as the "uncle bob" version requires users/maintainers to remember to call loadStuff() before they call getStuff() - this is a recipe for disaster if any single one of your maintainers forgets (or worse, never knew). Most of the places I've worked in the past decade are still using code that is more than a decade old, so ease of maintenance is a critical factor to consider.

Forget everyone's rules of thumb about what a get method should or should not do. A class should present an abstraction of something. Your class has readable stuff. In Java it is conventional to use 'get' methods to read properties. Billions of lines of frameworks have been written expecting to read stuff by calling getStuff. If you name your function fetchStuff or anything other than getStuff, then your class will be incompatible with all those frameworks.

You might point them to Hibernate, where 'getStuff()' can do some very complicated things, and throws a RuntimeException on failure.

Which just tends to make other code much more redundant and harder to read because you have to start wading through all of the similar calls. Additionally, calling loader functions or similar breaks the whole purpose of even using OOP in that you are no longer being abstracted away from the implementation details of the object you are working with. If you have a clientRoster object, you shouldn't have to care about how getNames works, as you would if you have to call a loadNames, you should just know that getNames gives you a List<String> with the names of the clients.

Thus, is sounds like the issue is more about semantics and the best name for the function to get the data. If the company (and others) has an issue with the get and set prefix, then how about calling the function something like retrieveNames instead? It says what is going on but doesn't imply that the operation would be instantaneous as might be expected of a get method.

In terms of logic in an accessor method, keep it to a minimum as they are generally implied to be instantaneous with only nominal interaction occurring with the variable. However, that also generally only applies to simple types, complex data types (i.e. List) I find harder to properly encapsulate in a property and generally use other methods for interacting with them as opposed to a strict mutator and accessor.

I don't completely agree on this. I agree that it should have no side effects, but I think it's perfectly fine to implement it in a way that differentiates it from a field. Looking at the .Net BCL, InvalidOperationException is widely used when looking at getters. Also, see MikeNakis answer on order-of-operations.
–
MaxDec 23 '11 at 11:43

1

@TMN: In a best case scenario the class should be organized in a way such that getters don't need to run operations capable of thowing exception. Minimizing the places that can throw exceptions leads to less unexpected surprises.
–
hugomgDec 23 '11 at 14:12

5

I'm going to disagree with the second point with a specific example: foo.setAngle(361); bar = foo.getAngle(). bar could be 361, but it might also legitimately be 1 if angles are bound to a range.
–
zzzzBovDec 23 '11 at 15:00

A getter which invokes other properties and methods in order to compute its own value also implies a dependency. Eg, if your property has to be able to compute itself, and doing so requires another member to be set, then you have to worry about accidental null-references if your property is accessed in initialization code where all members are not necessarily set.

That doesn't mean 'never access another member that isn't the properties backing field within the getter' it just means pay attention to what you are implying about what the required state of the object is, and if that matches the context you expect this property to be accessed in.

However, in the two concrete examples you gave, the reason I would choose one over the other is entirely different. Your getter is initialized on first access, eg, Lazy Initialization. The second example is assumed to be initialized at some prior point, eg, Explicit Initialization.

When exactly initialization occurs may or may not be important.

For example it could be very very slow, and needs to be done during a load step where the user is expecting a delay, rather than performance unexpectedly hiccuping when the user first triggers access (ie, user right clicks, , context menu appears, user has already right clicked again).

Also, sometimes there is an obvious point in execution where everything that can affect/dirty the cached property value occurs. You may even be verifying that none of the dependencies change and throwing exceptions later on. In this situation it makes sense to also cache the value at that point, even if it isn't particularly expensive to compute, just to avoid making the code-execution more complex and harder to follow mentally.

That said, Lazy Initialization makes a lot of sense in a lot of other situations. So, as often happens in programming its hard to boil down to a rule, it comes down to the concrete code.

Be careful about this, you can wind up exposing too much of your internal state. You don't want to wind up with a lot of empty loadFoo() or preloadDummyReferences() or createDefaultValuesForUninitializedFields() methods just because the initial implementation of your class needed them.
–
TMNDec 23 '11 at 13:21

A getter is just supposed to be a getter. So less logic could be more suitable. If you think generally it is a way to access a field or sth. else. Manupulation and validation, etc. are different jobs. Mine is just an idea about keeping them simple.

Personally, I would expose the requirement of Stuff via a parameter in the constructor, and allow whichever class is instantiating stuff to do the work of figuring out where it should come from. If stuff is null, it should return null. I prefer not to attempt clever solutions like the OP's original because it's an easy way to hide bugs deep inside your implementation where it's not at all obvious what might be going wrong when something breaks.

There are more important issues then just "appropriateness" here and you should base your decision on those. Mainly, the big decision here is wether you want to alow people to bypass the cache or not.

First, think about if there is a way to reorganize your code so all necessary load calls and cache management are done in the constructor/initializer. If this is possible you can create a class whose invariant allows you do to the simple getter from part 2 with the safety of the complex getter from part 1. (A win-win scenario)

If you cannot create such a class, decide if you have a tradeoff and need to decide wether you want to allow the consumer to skip the cache-checking code or not.

If it is important that the consumer never skip the cache check and you don't mind the performance penalties, then couple the check inside the getter and make it impossible for the consumer to do the wrong thing.

If it is OK to skip the cache check or it is very important that you are guaranteed O(1) performance in the getter then use separate calls.

As you might have already noted, I am not a big fan of the "clean-code", "split everything into tiny functions" philosophy. If you have a bunch of orthogonal functions that can be called in any order splitting them will give you more expressive power at little cost. However, if your functions have order dependencies (or are only really useful in a particular order) then splitting them only increases the number of ways you can do wrong things, while adding little benefit.

In my opinion, Getters should not have a lot of logic in them. They should not have side effects and you should never get an exception from them. Unless of course, you know what you're doing. Most of my getters have no logic in them and just go to a field. But the notable exception to that was with a public API that needed to be as simple as possible to use. So I had one getter that would fail if another getter hadn't been called. The solution? A line of code like var throwaway=MyGetter; in the getter that depended upon it. I'm not proud of it, but I still do not see a cleaner way to do it

This looks like a read from cache with lazy loading. As others have noted, checking and loading may belong in other methods. Loading may need to be synchronized so you don't get twenty threads all loading at the same time.

It might be appropriate to use a name getCachedStuff() for the getter as it won't have a consistent execution time.

Depending on how the cacheInvalid() routine works, the check for null may not be necessary. I wouldn't expect the cache to be valid unless stuff had been populated from the database.

IMHO it is very simple if you use a design by contract. Decide what should your getter provide and just code accordingly (simple code or some complex logic that may be involved or delegated somewhere).

The part of clean code that is relevant here is the clarity. Getters are typically used to transform data from it's internal representation to the caller's expected representation.

For example:

Player::getScores() could be internally represented as: a list of integers, a list of decimals, or a string containing a delimited list of scores, calling this method should not log onto Steam, play 20 games of Pac-Man and then return the scores.

Programmers also expect to be able to loop through 100 players while calling getScores() for each of them. Now this causes 2000 games of Pac-Man, if you hide this information, programmers will be unsure of what a getter actually means in your code, which defeats the purpose of using getters at all.

At the very least, using a different name means that the method will not be confused for a getter.