Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

This question has been asked in different ways, but nothing that's actually what I want to know.

Got me confused at work now. They 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();
}

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

share|improve this question
3  
This looks more like tryGetStuff() to me... –  Bill Michell Dec 23 '11 at 9:46
4  
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 Yankov Dec 27 '11 at 20:00
    
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 Hanna Jan 24 at 17:36
add comment

14 Answers

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.

share|improve this answer
11  
+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 Troller Dec 23 '11 at 15:37
2  
if it can fail - it's not a getter. In this case what if the DB link is down? –  Martin Beckett Dec 23 '11 at 15:58
    
@Martin Beckett that's a valid concern, and you should have written an answer where we could discuss it. I think that with exceptions, and with Java's ability to publicly declare that a method may throw (see throws keyword) failability is not such a big deal in determining whether something should or should not be a getter. But I may be wrong. –  Mike Nakis Dec 23 '11 at 16:05
1  
+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. –  Izkata Dec 23 '11 at 17:20
show 2 more comments

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.

share|improve this answer
add comment

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

share|improve this answer
6  
-1. The real headache will be when the caller is stuck figuring out why a simple getter call resulted in a slow-as-hell database access. –  Domenic Dec 23 '11 at 14:23
6  
@Domenic: The database access has to be done anyway, you're not saving anyone performance by not doing it. If you need this List<Stuff>, there's only one way to get it. –  DeadMG Dec 23 '11 at 14:28
1  
according to clean code you should write if (hasStuff()) { return stuff; } lodadStuff(); return stuff; –  lukas Dec 23 '11 at 19:39
1  
@lukas: Thanks, I didn't remember all the tricks used in 'Clean' Code to make trivial bits of code yet one line longer ;-) Fixed now. –  Joonas Pulakka Dec 24 '11 at 8:19
add comment

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.

share|improve this answer
add comment

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
share|improve this answer
2  
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. –  Max Dec 23 '11 at 11:43
    
Agree with all points except the last one. It's certainly possible that getting a value may involve performing a calculation or some other operation that depends on other values or resources which may not have been set. In those cases I would expect the getter to throw some kind of exception. –  TMN Dec 23 '11 at 13:14
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. –  missingno Dec 23 '11 at 14:12
    
+1, this is the best answer. Point 2 is huge and often missed. Point 3 is very important as well. I agree on point 4, despite some counterarguments from other commenters; I think if a getter is throwing an InvalidOperationException then the API should have been designed differently, so as not to expose that property as a getter. It even looks weird: try { var x = myFoo.Bar; } makes no sense---what are you trying to do?---whereas try { var x = myFoo.FetchBar(); } says that you're trying to fetch bar. –  Domenic Dec 23 '11 at 14:27
3  
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. –  zzzzBov Dec 23 '11 at 15:00
add comment

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.

share|improve this answer
add comment

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

share|improve this answer
    
+1: I agree with you! If the object is just meant to hold some data, then the getters should only return the current content of the object. In this case it is some other object's responsibility to load the data. If the contract says that the object is the proxy of a database record, then the getter should fetch the data on the fly. It can get even more complicated if the data has been loaded but is not up to date: should the object be notified of changes in the database? I think there is no unique answer to this question. –  Giorgio Jan 4 '12 at 15:35
add comment

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.

share|improve this answer
add comment

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

share|improve this answer
1  
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. –  TMN Dec 23 '11 at 13:21
    
Sure... I was just telling that if you do what the name states that there shouldn't be many problems... but what you say is absolutly true... –  Ivan Crojach Karačić Dec 23 '11 at 14:11
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
    
-1, constructors should construct, not initialize. Putting database logic in a constructor makes that class completely un-testable, and if you have more than a handful of these your application startup time will become immense. And that's just for starters. –  Domenic Dec 23 '11 at 14:28
    
@Domenic: This is a semantic and language-dependent issue. The point that an object is fit to use and provides the appropriate invariants after, and only after, it is fully built. –  missingno Dec 23 '11 at 16:30
add comment

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

share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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