Which is more appropriate: getters and setters or functions?

StackOverflow https://stackoverflow.com/questions/175228

Question

Is it ever appropriate to abandon the "getMyValue()" and "setMyValue()" pattern of getters and setters if alternative function names make the API more obvious?

For example, imagine I have this class in C++:


public class SomeClass {
private:
    bool mIsVisible;

public:
    void draw();
    void erase();
}

I could add functions to get/set "mIsVisible" like this:


bool getVisible() { return mIsVisible; };



void setVisible(bool visible) { 
    if (!mIsVisible && visible) {
        draw();
    } else if (mIsVisible && !visible) {
        erase();
    }



mIsVisible = visible;




}

However, it would be equally possible to use the following methods instead:


bool isVisible() { return mIsVisible; };

void show() { 
    if (!mIsVisible) {
        mIsVisible = true;
        draw();
    }
}

void hide() {
    if (mIsVisible) {
        mIsVisible = false;
        erase();
    }
}

In brief, is it better to have a single "setVisible(bool)" method, or a pair of "show()" and "hide()" methods? Is there a convention, or is it purely a subjective thing?

Was it helpful?

Solution

Have a read of the article "Tell, Don't Ask" over at the Pragmatic Programmers web site and I think you'll see that the second example is the way to go.

Basically, you shouldn't be spreading the logic out through your code which is implied with your first example, namely:

  1. get current visibility value,
  2. make decision based on value,
  3. update object.

OTHER TIPS

In the example you gave, show() and hide() make a lot of sense, at least to me.

On the other hand, if you had a property skinPigment and you decided to make functions called tanMe() and makeAlbino() that would be a really poor, non-obvious choice.

It is subjective, you have to try to think the way your users (the people utilizing this class) think. Whichever way you decide, it should be obvious to them, and well-documented.

I would go with the isVisible()/show()/hide() set.

setVisible() implies that all it does it change the internal variable. show() and hide() make the side effects clear.

On the other hand, if all getVisible()/setVisible() did was to change the internal variable, then you've just changed remarkably little from having them as public fields.

setters actually have very little to do with object orientation, which is the programming idiom applied in the example. getters are marginally better, but can be lived without in many cases. If everything can be gotten and set, what's the point of having an object? Operations should be called on objects to accomplish things, changing the internal state is merely a side-effect of this. The bad thing about a setter in the presence of polymorphism - one of OO's cornerstones - is that you force every derived class to have a setter. What if the object in question has got no need for an internal state called mIsVisible? Sure he can ignore the call and implement as empty, but then you are left with a meaningless operation. OTOH, operations like show and hide can be easily overridden with different implementations, without revealing anything about the internal state.

In general, I think setters/getters should only set the values of properties. In your example, you are also performing an action based on the value of the isVisible property. In this case, I would argue that using functions to perform the action and update the state is better than having a setter/getter that performs an action as a side-effect of updating the property.

If switching mIsVisible really turns visibility of the object on and off immediately, than use the show/hide scenario. If it will stay in the old state a little longer (e.g. until something else triggers a redraw) then the set/get scenario would be the way to go.

I prefer the show() and hide() methods because they explicitly tell what you are going. The setVisible(boolean) doesn't tell you if the method is going to show/draw right away. Plus show() and hide() are better-named method for an interface (IMHO).

Implicitly, the 'show' and 'hide' functions you list are both setters

For booleans, I'd think that a single tool like you've shown would be good. However, a .show and .hide function also look like commands, not functions that change the state of the object.

In the case you actually have to write code like 

if (shouldBeShowingAccordingToBusinessLogic()) w.show();
else w.hide();

all over the place, you might be better off with

w.showIfAndOnlyIf(shouldBeShowingAccordingToBusinessLogic())

Or, for truly bizarre cases, when your logic can't decide whether to dhow or not till the end of some code stretch, you can try

w.setPostponedVisibility(shouldBeShowingAccordingToBusinessLogic());
...
w.realizeVisibility();

(Didn't I say it's bizzare?)

An additional motivation to go for the display/hide solution is that as a setter,

the setVisible method has a 'side effect', in that it also displays or hides SomeClass. The display/hide methods better convey the intent of what happens.

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top