C++ Get/Set accessors - how do I avoid typing repetitive code?

Question

I'm writing a pretty large library, and I find myself writing almost identical accessors all the time. I already have several dozen accessors such as the one below.

Question: How can I declare/implement accessors to save typing all this repetitive code? (No #defines please; I'm looking for C++ constructs.)

Update: Yes, I do need accessor functions, because I need to take pointers to these accessors for something called Property Descriptors, which enable huge savings in my GUI code (non-library).

.h file

private:
    bool _visible;
public:
    bool GetVisible() const { return _visible; }
    void SetVisible (bool value);

// Repeat for Get/SetFlashing, Get/SetColor, Get/SetLineWidth, etc.

.cpp file

void Element::SetVisible (bool value)
{
    _visible = value;
    this->InvalidateSelf(); // Call method in base class
    // ...
    // A bit more code here, identical in 90% of my setters.
    // ...
}

// Repeat for Get/SetFlashing, Get/SetColor, Get/SetLineWidth, etc.

Answer 1

I find myself writing almost identical accessors all the time. I already have several dozen accessors such as the one below.

This is a sure design smell that you are writing accessors "for the sake of it". Do you really need them all? Do you really need a low-level public "get" and "set" operation for each one? It's unlikely.

After all, if all you're doing is writing a getter and a setter for each private data member, and each one has the same logic, you may as well have just made the data members public.

Rather your class should have meaningful and semantic operations that, in the course of their duties, may or may not make use of private data members. You will find that each of these meaningful operations is quite different from the rest, and so your problem with repetitive code is vanquished.

As n.m. said:

Easy: avoid accessors. Program your classes to do something, rather than have something.

Even for those operations which have nothing more to them, like controlling visibility, you should have a bool isVisible() const, and a void show(), and a void hide(). You'll find that when you start coding like this it will promote a move away from boilerplate "for the sake of it" getters & setters.

Answer 2

Whilst I think Lightness Races in Orbit makes a very good point, there is also a few ways that can be used to implement "repeating code", which can be applied, assuming we do indeed have a class that have "many things that are similar that need to be controlled individually, so kind of continuing on this, say we have a couple of methods like this:

void Element::Show()
{
   visible = true;
   Invalidate();
   // More code goes here. 
}

void Element::Hide()
{
   visible = false;
   Invalidate();
   // More code goes here. 
}

Now, to my view, this breaks the DRY (Do not Repeat Yourself) principle, so we should probably do something like this:

void Element::UpdateProperty(bool &property, bool newValue)
{
   property = value;
   Invalidate();
   // More code goes here. 
}

Now, we can implement Show and Hide, Flash, Unflash, Shaded etc by doing this, avoiding repetition inside each function.

void Element::Show()
{
    UpdateProperty(visible, true);
}

If the type isn't always bool, e.g. there is a position, we can do:

template<typename T>void Element::UpdateProperty(T &property, T newValue)
{
   property = value;
   Invalidate();
   // More code goes here. 
}

and the MoveTo becomes:

void Element::MoveTo(Point p)
{
    UpdateProperty(position, p);
}

Edit based on previously undisclosed information added to question:

Obviously the above technique can equally be applied to any form of function that does this sort of work:

void Element::SetVisible(bool value)
{
   UpdateProperty(visible, value);
}

will work just as well as for Show described above. It doesn't mean you can get away from declaring the functions, but it reduces the need for code inside the function.

Answer 3

I agree with Lightness. You should design your classes for the task at hand, and if you need so many getters and setters, you may be doing something wrong.

That said, most good IDEs allow you to generate simple getters and setters, and some might even allow you to customize them. You might save the repetitive code as a template and select the code fragment whenever needed.

You may also use a customizable editor like emacs and Vim (with Ultisnips) and create some custom helping functions to make your job easy. The task is ripe for automation.

Answer 4

The only time you should ever write a get/set set of functions in any language is if it does something other than just read or write to a simple variable; don't bother wrapping up access to data if all you're doing is make it harder for people to read. If that's what you're doing, stop doing anything.

If you ever do want a set of get/set functions, don't call them get and set -- use assignment and type casting (and do it cleverly). That way you can make your code more readable instead of less.

This is very inelegant:

class get_set {
    int value;
public:
    int get() { return value; }
    void set(int v) { value = v; }
};

This is a bit better

class get_set_2 {
     value_type value;
     bool needs_updating;
 public:
    operator value_type const & () {
        if (needs_updating) update(); // details to be found elsewhere
        return value;
    }

    get_set_2& operator = (value_type t) {
        update(t); // details to be found elsewhere
        return *this;
    }
};

If you're not doing the second pattern, don't do anything.

Answer 5

I'm a tad late again, but I wanted to answer because I don't totally agree with some other here, and think there's additional points to lay out.

It's difficult to say for sure if your access methods are code smells without seeing a larger codebase, or have more information about intent. Everyone here is right about one thing: access method are generally to be avoided unless they do some 'significant work', or they expose data for the purpose of generic-ism (particularly in libraries).

So, we can go ahead and call methods like the idiomatic data() from STL containers, 'trivial access method'.

Why not use trivial access methods?

First, as others have noted, this can lead to an over-exposure of implementation details. At it's best such exposure makes for tedious code, and at it's worse it can lead to obfuscation of ownership semantics, resource leaks, or fatal exceptions. Exposure is fundamentally opposite of object orientation, because each object ought to manage its own data, and operations.

Secondly, code tends to become long, hard to test, and hard to maintain, as you have noted.

When to use trivial access methods?

Usually when their intent is specific, and non-trivial. For example, the STL containers data() function exists to intentionally expose implementation details for the purposes of genericism for the standard library.

Procedural style-structs

Breaking away from directly object-oriented styles, as implementation sometimes does; you may want to consider a simple struct (or class if you prefer) which acts as a data carrier; that is, they have all, or mostly, public properties. I would advise using a struct only for simple holders. This is opposed to a class ought to be used to establish some invariant in the constructor. In addition to private methods, static methods are a good way to illustrate invariants in a class. For example, a validation method. The invariant establishment on public data is also very good for immutable data.

An example:

// just holds some fields
struct simple_point {
    int x, y;
};

// holds from fields, but asserts invariant that coordinates
// must be in [0, 10].
class small_point {
  public:
    int x, y;

    small_point() noexcept : x{}, y{} {}
    small_point(int u, int v) 
    {
        if (!small_point::valid(u) || !small_point::valid(u)) {
           throw std::invalid_argument("small_point: Invalid coordinate.");
        }

        x = u;
        y = v;
    }

    static valid(int v) noexcept { return 0 <= v && v <= 10; }
};