How Much Logic in Getters

Question

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

Yet, I am convinced that a lot of stuff can be hidden in getters and setters to shield users/programmers from implementation details.

An example of what I do:

public List<Stuff> getStuff()
{
   if (stuff == null || cacheInvalid())
   {
       stuff = getStuffFromDatabase();
   }
   return stuff;
}

An example of how work tells me to do things (they quote 'Clean Code' from Uncle Bob):

public List<Stuff> getStuff()
{
    return stuff;
}

public void loadStuff()
{
    stuff = getStuffFromDatabase();
}

How much logic is appropriate in a setter/getter? What's the use of empty getters and setters except a violation of data hiding?

Answer 1

The way work tells you to do things is lame.

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.

Answer 2

Logic in getters is perfectly fine.

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.

Answer 3

I think that according to 'Clean Code' it should be split as much as possible, into something like:

public List<Stuff> getStuff() {
   if (hasStuff()) {
       return stuff;
   }
   loadStuff();
   return stuff;
}

private boolean hasStuff() {
    if (stuff == null) {
       return false;
    }
    if (cacheInvalid()) {
       return false;        
    }
    return true;
} 

private void loadStuff() {
    stuff = getStuffFromDatabase();
}

Of course, this is complete nonsense, given that the beautiful form, which you wrote, does the right thing with a fraction of code that anyone understands at a glance:

public List<Stuff> getStuff() {
   if (stuff == null || cacheInvalid()) {
       stuff = getStuffFromDatabase();
   }
   return stuff;
}

It shouldn't be the caller's headache how the stuff is got under the hood, and particularly it shouldn't be the caller's headache to remember to call things in some arbitrary "right order".

Answer 4

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.

Answer 5

You are right, your colleagues are wrong.

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.

Answer 6

Sounds like this might be a bit of a purist versus application debate that might be affected by how you prefer to control function names. From the applied standpoint, I would much rather see:

List<String> names = clientRoster.getNames();
List<String> emails = clientRoster.getEmails();

As opposed to:

myObject.load();
List<String> names = clientRoster.getNames();
List<String> emails = clientRoster.getEmails();

Or even worse:

myObject.loadNames();
List<String> names = clientRoster.getNames();
myOjbect.loadEmails();
List<String> emails = clientRoster.getEmails();

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.

Answer 7

Calling a getter should exhibit the same behavior as reading a field:

• It should be cheap to retrieve the value
• If you set a value with the setter and then read it with the getter, the value should be the same
• Getting the value should have no side-effects
• It should not throw an exception

Answer 8

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.

Answer 9

Just do it as @MikeNakis said... If you just get the stuff then it's fine... If you do something else create a new function that does the job and make it public.

If your property/function is doing only what it's name says then there isn't much room for complication left. Cohesion is key IMO

Answer 10

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.

Answer 11

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.

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

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

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

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

Answer 12

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

Answer 13

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.

Answer 14

The main logic I'd expect to see in getters that return a list is logic to make sure the list is unmodifiable. As it stands both of your example potentially break encapsulation.

something like:

public List<Stuff> getStuff()
{
    return Collections.unmodifiableList(stuff);
}

as for caching in the getter, I think this would be OK but I might be tempted to move out the cache logic if building the cache took a significant time. i.e. it depends.

Answer 15

Depending on the exact use case, I like to seperate out my caching into a separate class. Make a StuffGetter interface, implement a StuffComputer which does the calculations, and wrap it inside an object of StuffCacher, which is responsible for either accessing the cache or forwarding calls to the StuffComputer that it wraps.

interface StuffGetter {
     public List<Stuff> getStuff();
}

class StuffComputer implements StuffGetter {
     public List<Stuff> getStuff() {
         getStuffFromDatabase()
     }
}

class StuffCacher implements StuffGetter {
     private stuffComputer; // DI this
     private Cache<List<Stuff>> cache = new Cache<>();

     public List<Stuff> getStuff() {
         if cache.hasStuff() {
             return cache.getStuff();
         }

         List<Stuffs> stuffs = stuffComputer.getStuff();
         cache.store(stuffs);
         return stuffs;
     }
}

This design lets you easily add caching, remove caching, change the underlying derivation logic (e.g. accessing a DB vs returning mock data), etc. It's a bit wordy, but it's worth while for sufficiently advanced projects.

Answer 16

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