C.131: Please reconsider this rule; trivial getters and setters can make sense

[GiovanniDicanio]

Rule C.131 "Avoid trivial getters and setters" sounds like a wrong piece of advice to me, unless I'm missing something.

In fact, I think there are cases in which "trivial" getters and setters do make sense. For example, considering the Point class case, it's good to have ("trivial") getters and setters instead of directly exposing coordinate data members. In fact, if someone later changes the implementation details of the Point class, e.g changing the coordinate system from (x,y) cartesian system to a polar coordinate system, users of the Point class can still call getX() and getY() to retrieve the desired X and Y coordinate of a Point instance. This is in my opinion a good example of information hiding and development of maintainable code.

So, I'm open to be proven wrong on that, but I think this rule should be seriously reconsidered.

Thanks.

[rianquinn]

I think the rule is trying to show the difference between when to use a struct, and when to use a class, and there are certainly places where no API change is expected, and no mocking is needed.

That being said, I agree that a trivial getter / setter provides a means to change the implementation underneath without having to change the API itself, which should be encouraged, not discouraged. You see this a lot in Qt (QPoint, QRect, etc... being a good example). Also... I don't know of an easy way to mock a variable, so if you do any unit testing (which should be everyone), likely this rule by it's very nature would have to be ignored (how do you verify a variable that has been modified without exposing the implementation?)

[Dave-Lowndes]

While I agree that trivial getters and setters are often pointless, and also can make analysing code more difficult, I don't think it warrants a core check because as Giovanni points out, the implementation can change. You could also have a public getter and a private (or no) setter. Additionally, a debug build may have diagnostic checks that are absent in a release build. If the getters and setters are trivial, it's an optimizers job to ensure generated code is efficient.

[cubbimew]

The point of getters, IMO, is to provide just enough information to efficiently identify every possible value an object can hold, not to access individual data members (which is the case where they break encapsulation and the reason they look so suspicious). For a non-Riemann 2D point, yes, cartesian coordinates are a natural way to differentiate between values and also are a natural implementation technique, so "trivial" getters (not setters) make sense there... no idea how to reflect that with enforcement though. Annotate "trivial on purpose"?

[rianquinn]

@cubbimew Due to the inability to mock a trivial getter, I still disagree. More specifically, this should not be a rule that is checked, enforced, or used as a comment during a review as there is nothing wrong (and is really required) to implement something as simple as a point with setters / getters. It's also perfectly valid to not have the setters / getters (as you state) if that works for your situation (we have an example of this in our hypervisor for the GDT / IDT registers as well as the TSS). Which begs the question should the C++ Core Guidelines really be explicitly stating this? I lean on the side of no, because in most cases, people will have to unit test their code, in which case getters / setters are needed for mocking.

[cubbimew]

I am not sure I follow the testing argument. std::vector's size() and capacity() are not trivial getters (in prevalent implementations). How would its testability be improved if it additionally had get_end_pointer() and get_end_of_capacity_pointer()?

[rianquinn]

@cubbimew I'm not sure I follow your example, but for the purpose of this thread, a "point" has been used so I'll at least talk to that.

Lets assume the following:

struct point
{
    int x;
    int y;
}

Now, pass this to a class that performs an operation but doesn't return a value (a very common case where mocking is needed). There is no way to verify that x and y have been modified as expected. If you use getters / setters (even if all they are doing is trivially wrapping the variables) you can now use your favorite mocking engine (mine is hippmocks) to mock point and observe it being changed as expected. Under the hood, the mocking engines (most that I have seen at least) overwrite your PLT entries for the setters / getters with functions that you define which perform the observation, which cannot be done with a trivial variable.

My point is, since this is a common scenario when unit testing, this rule makes little sense as both forms (having setters / getters and not having them) are valid, and thus advising people to use one form over another without the context of the problem at hand is just misleading. IMO this rule really should be written the other way to say, in most cases, getters / setters should be preferred unless you know for sure that the implementation will never change, and unit testing will never be needed by the user of the struct, and not the struct itself.

[gdr-at-ms]

There is no way to verify that x and y have been modified as expected.

Why?

[rianquinn]

@gdr-at-ms Because point has been modified by something that doesn't output anything to verify. For more information on mocking, please see here and here

[gdr-at-ms]

I don't get it. If point has been modified and you can't verify that, then your problem isn't that you don't have trivial setter and getters. It is a framework problem.

[rianquinn]

@gdr-at-ms It's not a framework problem. Mocking is standard practice in TDD, and to be honest, I don't think the C++ Core Guidelines thread is a good place to be lecturing about TDD.

[gdr-at-ms]

"standard practice" does not mean you don't have a framework problem. A lot of the rules in the Core Guidelines go against things that someone somewhere considers "standard practice".

One of the goals of the Core Guidelines is help C++ programmers write good C++, adopt solid and robust practices. We feel that trivial setters and getters are to be avoided. This is based on decades on experience, writing code, maintaining code written by others, reviewing code. It isn't lecturing anybody who disagrees with those insights.

[MikeGitb]

@rianquinn: I don't really have an opinion on the guideline, but why can't you observe the point's state after the function returns? And if your function only modifies an internal copy whose modification cannot be observed outside, does it really matter?

[rianquinn]

Please read the links as it answers the questions in better detail as to why mocking is used.

@gdr-at-ms IMO, good C++ code is code that can be tested, and writing a rule the gets in the way of TDD doesn't make much sense when the rule doesn't provide good context, and it alienates a lot of people. I guess a good question is, why does this rule exist at all... what's so bad about getters / setters when the compiler will just get rid of them anyways (especially if you inline them). Seems to me this rule does more harm than good.

@MikeGitb As an example, lets take a paint engine who moves a cursor around for drawing. The cursor is an internal thing that stores a point and it's cord system is implementation specific (not part to the exposed API). Now lets say the API provides a means to draw a line using two points. A unit test of the paint engine will want to verify that the cursor is placed in specific locations, but there is no way to query via the public API about the cursor location as this is implementation specific. A mock of the cursor can provide a simple "seem" to verify the unit test without exposing the API. This is a pretty overly simplistic example, and a lot of unit testing books provide better examples of how to use mocking to verify an implementation when no public API is available to observe.

Like I said, I don't want to argue about TDD on this thread, and I think the original statement that the implementation could change alone is enough of an argument to say that setters / getters should be used, especially when I don't see in the guidelines a reason why they are bad.

[gdr-at-ms]

@rianquinn We are assuming that you're reading what we write and that you are giving it due consideration. Please, return the favor and don't assume the contrary until you have evidence.

Nothing I write question the "use of mock", nor against "testing". Rather I stated there is a framework problem.

[rianquinn]

@gdr-at-ms I know how to read, I always give comments due consideration, and I assume nothing.

There is no issue with mocking frameworks like hippomocks or gmock (Google's mocking framework). It's impossible to mock a variable (just like it's impossible to mock an inline function with optimizations enabled) because the compiler leaves nothing to hook. If anything it's an issue with C++ (for example, mocking usually requires functions being labeled as virtual purely for the sake of testing, which is ugly) as other languages don't have this same issue.

All I'm saying is... I see no point to having this rule explicitly defined as I don't see the benefit, and a couple of us have at least laid out some examples of why this rule is not a good one. The rule itself doesn't provide a reason for it's existence, unlike other rules which provide explanations.

[galik]

I have gone back and forth over the getters and setters issue quite a lot. It seems to me the heart of the guideline C.131 is about treating data like data and class objects like class objects.

We don't see much of this happening:

class integer
{
public:
    int get() const { return i; }
    void set(int i) { this->i = i; }

private:
    int i;
};

Even though it would allow us to change the integer to a float and keep the original interface and even though it would allow us to mock the integer.

Sometimes we just want to deal with data as in:

int i;

There are also times when our plain data needs to be aggregated so we can process an int and a float and a std::string as a unit of data rather like our int:

struct record
{
    std::string id;
    int age;
    float weight;
};

If every time we treat data and aggregate data as full blown class objects with mocking and the possibility of changing the interface, then the quantity of code we would be producing would itself impose a burden of bugs due to the fact that there will inevitably be typos creeping into the works like this:

class record
{
public:
    std::string get_id() const { return id; }
    void set_id(std::string const& id) { this->id = id; }

    int get_age() const { return age; }
    void set_age(int age) { this->age = age; }

    float get_weight() const { return weight; }
    void set_weight(int weight) { this->weight = weight; } // oops cut'n'paste error

private:
    std::string id;
    int age;
    float weight;
};

Now we have a potentially nasty hard to spot error on our hands. The more code you write, the more bugs you write.

Another point is that some people believe that an awful lot of what people do for unit testing (the majority of it?) is somewhat redundant and possibly even counter productive. If the quantity of effort needed to ensure a single value is large then maybe it's worth considering whether or not it is too much?

I suspect it is quite possible that coarser grained "black box" unit testing can be equally or even more effective than following every member of every struct of aggregate data.

And I often hear the argument for using getters and setters because of interface changes. But I tend to think such interface changes are relatively rare. How many times have you really changed code that could have been simpler if getters and setters had been used instead or basic types? If the type of a field changes from int to float are you really still going to want the int get() laying around?

Perhaps the real problem is people thinking in terms of data rather than abstract objects. If you have lots of getters and setters for your data members then you are not encapsulating correctly and the way you are thinking about your objects is possibly more procedural than you might like.

You can separate data for procedural processing from (object orientated) abstract data by reducing the number of getters and setters. Agregate data should be treated like data and abstract data should be treated according to its external behavior as a whole entity, not its internal components.

[rianquinn]

@galik I 100% agree with your argument here, and in our code we have examples of all of these, which gets to my point. Your example provides context, which a Linter will not have. Putting a rule like this in the Guidelines will cause the tool to say "get rid of the function and publicly expose the variable" every time it sees you providing a simple getter / setter even if you have a good reason for it. I don't think there is a good argument to generally say, don't wrap your variables as it entirely depends on your use case.

I will add that the example you gave above should be caught by "-wconversion", but that's me being pedantic. 😄 Also, generic "always auto" programming habits should also get rid of such issues, and an example of that could also be provided if the major concern is unanticipated type conversions.

[vcato]

I do feel like there should be some guideline related to the over-use of getters and setters, but saying "avoid" them seems a bit strong. Would it perhaps be better to say that you should avoid classes whose only member functions are trivial getters and setters?

[rianquinn]

@vcato Agreed. I think there already is a rule for the use of struct vs class that basically says this. I might be mistaken.

[rianquinn]

Another good example of how a Linter would have trouble with this is with C.133, although I will admit that this rule is not complete, the end result would likely be a trivial getter / setter.

I think one resolution to this issue might be to simply add "(Not enforceable)" just like with I.8. This way, the advise is still there, but there is no enforcement via a Linter which will not know if the trivial getter / setter is there for a good reason (i.e. hooking, to be replaced in the future by design, or private access to derivations). At least that seems like an appropriate compromise.

[AndrewPardoe]

Per our discussion, there is not consensus in the C++ community about setters and getters. Members of the community and the committee repeatedly discuss this issue, one example here, however, and even some amongst the editors have disagreement about their utility.

We're deferring to make a change at this time. This rule specifically addresses trivial getters and setters--getters and setters that convert from internal to interface types are not trivial and can be useful--this is an existing note in the rule.