Getter for enum member returns a copy of enum?

[ElusiveTau]

I came across something non-intuitive, found a solution, but don't fully understand why the solution works.

#include <iostream>

using namespace std;

enum ConnectionStatus
{
	Disconnected = 0, 
	Connected
};

class Status
{
public:
	inline ConnectionStatus getConnectionStatus()
	{
		return connectionStatus;
	}

	inline void setConnectionStatus(ConnectionStatus val)
	{
		connectionStatus = val;
	}

private:
	ConnectionStatus connectionStatus = ConnectionStatus::Disconnected;
};

class Foo
{
public: 
	inline Status getStatus() 
	{
		return status;
	}

	inline void setStatus(Status val)
	{
		status = val;
	}

private:
	Status status;
};

int main()
{
	Foo f; 
	cout << "Init Connection Status: " << f.getStatus().getConnectionStatus() << endl;
	f.getStatus().setConnectionStatus(ConnectionStatus::Connected); 
	cout << "After setting status: " << f.getStatus().getConnectionStatus() << endl;

	return 0;
}

Init Connection Status: 0
After setting status: 0

After calling f.getStatus().setConnectionStatus(Connected), I'd expect f.status.connectionStatus to be ConnectionStatus::Connected but it's still ConnectionStatus::Disconnected!

Is it because getStatus(), in the call f.getStatus().setConnectionStatus(Connected), returns-by-value a copy of f.status and .setConnectedStatus(Connected) sets the .connectionStatus of the copy, rather than of f.status?

How could we show that .setConnectedStatus() is invoked from the copied Status object, rather than from f.status?

This declaration produces the desired behavior:

#include <iostream>

using namespace std;

enum ConnectionStatus
{
	Disconnected = 0, 
	Connected
};

class Status
{
public:
	inline ConnectionStatus getConnectionStatus() // [1]
	{
		return connectionStatus;
	}

	inline void setConnectionStatus(ConnectionStatus val)
	{
		connectionStatus = val;
	}

private:
	ConnectionStatus connectionStatus = ConnectionStatus::Disconnected;
};

class Foo
{
public: 
	inline Status& getStatus()  // Return a REFERENCE to this->status
	{
		return status;
	}

	inline void setStatus(Status val)
	{
		status = val;
	}

private:
	Status status;
};

int main()
{
	Foo f; 
	cout << "Init Connection Status: " << f.getStatus().getConnectionStatus() << endl;
	f.getStatus().setConnectionStatus(ConnectionStatus::Connected); 
	cout << "After setting status: " << f.getStatus().getConnectionStatus() << endl;

	return 0;
}

Should the getter on line [1] return a reference to ConnectionStatus as well? I guess the core issue here is that the getter's signature is incorrect and should return a reference.

[coder777]

Don't make your life harder than necessary. When you have both setter and getter your member variable is basically public just with overhead.
So I suggest to remove the getter/setter overhead and make the member public.

Should the getter on line [2] return a reference to ConnectionStatus as well?

No. There's nothing useful you can do with such a reference.

[kigar64551]

Should the getter on line [1] return a reference to ConnectionStatus as well?

In most cases, a "getter" function should not return a non-const reference, because leaking a non-const references to "internal" data is usually undesired. It would allow the caller to modify "internal" data (via the reference) without using the "setter" function!

Returning a const reference from a "getter" function can make sense, e.g. if the data to be return is big and you don't want to return a copy. But, in this case, you are just returning a simple enum value (eseentially an int), so returning a reference doesn't make much sense.

In cases where you do return a non-const reference from your "getter" function – typically a reference to some sort of "nested" object that you explicitly do want the caller to be able to modify – a separate "setter" function is not needed and wouldn't make much sense!

I think you should change your code like this:

enum ConnectionStatus
{
	Disconnected,
	Connected
};

class Status
{
public:
	inline ConnectionStatus getConnectionStatus() const // <-- return current status "by value"
	{
		return connectionStatus;
	}

	inline void setConnectionStatus(const ConnectionStatus val)
	{
		if ((val != Connected) && (val != Disconnected)) { 
			std::abort(); // <-- input validation: protect us from "rogue" value
		}
		connectionStatus = val;
	}

private:
	ConnectionStatus connectionStatus = ConnectionStatus::Disconnected;
};

class Foo
{
public:
	inline Status &getStatus() // <-- return mutable (non-const) reference to "inner" Status object
	{
		return status;
	}

private:
	Status status;
};

int main()
{
	Foo foo;
	std::cout << "Init Connection Status: " << foo.getStatus().getConnectionStatus() << std::endl;
	foo.getStatus().setConnectionStatus(ConnectionStatus::Connected);
	std::cout << "After setting status: " << foo.getStatus().getConnectionStatus() << std::endl;
	// foo.getStatus().setConnectionStatus((ConnectionStatus)42);
	return 0;
}

Works as expected:

Init Connection Status: 0
After setting status: 1

Don't make your life harder than necessary. When you have both setter and getter your member variable is basically public just with overhead.

Well, that's debatable. Many people think it's a good practice to use "getter" and "setter" functions in the public interface of your class, even if they do not contain any additional logic yet, and keep your member variables private. For example, you may want to add input validation to your "setter" function(s) at a later time (e.g. to ensure that only "valid" data will be written into the member variable), and that would not be possible if the member variables were public. Making the member variable(s) private later potentially breaks a lot of existing code...

(As for the performance, the compiler is almost certainly going to inline the getter/setter code anyway, even without the inline directive)

[Duthomhas]

Yes, when you have a specific object instance you are working over, particularly for one maintaining some resource like a file or a connection, then yes, you should return a reference to that object.
Otherwise you end up splicing its state between different objects.

BUT!
Be sure to distinguish between stateful objects and things without state, like enumeration values. An enumeration is just an integer.

coder777 wrote:

I suggest to remove the getter/setter overhead and make the member public.

It depends. If setting a value has side-effects (such as actually changing a connection status), then it should be public accessors.
And even if it is just a way to get at a simple mutable value, having a public API using accessors makes adding side-effects later easy and stablizes your API.

However, if the status is only useful as an object to query a connection’s status, then it should be read-only — which again is controlled via a getter method.


Code Structure

Personally, I do not like the structure of the code. You do not need a separate “status” object — just an object that connects. Querying its status should be part of its public interface.
It is the separating out the “status” as a distinct object is the cause of the problem, not per-se choices of reference-vs-value.

A better structure would be something like:

struct ConnectedThing
{
  enum class ConnectionStatus
  {
    Disconnected,
    Connected
  };

  virtual ConnectionStatus status() const;

  virtual ConnectedThing & connect( const std::string & );
  virtual void             disconnect();
};

struct HTTPServer : public ConnectedThing
{
  ConnectionStatus status() const
  {
    return _server_fd < 0 
      ? ConnectionStatus::Disconnected 
      : ConnectionStatus::Connected;
  }

  ...etc...

private:
  int _server_fd;
};

Here we create an “interface type”, then a concrete class that implements that interface. The interface defines a read-only value indicating the status: connected or not-connected.

Does that make sense?

[seeplus]

Is it because getStatus(), in the call f.getStatus().setConnectionStatus(Connected), returns-by-value a copy of f.status and .setConnectedStatus(Connected) sets the .connectionStatus of the copy, rather than of f.status?

Yes.

[jonnin]

I realize this code may be just an example to demonstrate for a question and study purposes. But if you are crafting a program with this kind of monstrosity in it, I beg you to reconsider.
You have spent 40 lines or so making a beautiful OOP complication that will propagate through your code like wildfire... and it will accomplish exactly the same result if the user of the class simply had inside of it:

someuserclass device
{
...
bool isconnected{false}; // maintain this?
...
};

...
//possible uses something like
device thing;
thing.connect(remote); //only way to set it true is to connect to something successfully
thing.disconnect(); //only set it false directly.


if your status had a lot more possible values, an enum, enum-class, or something like what you have becomes more and more attractive. So the thinking isn't "wrong" but its overengineered for a binary condition. I strongly dislike the enum class c++ provided, and am more likely to do just about anything else at all than use it, but its there, and its probably expected in a heavily policed code base.

[keskiverto]

Isn't the enum a red herring here? The main question was the copy in:

Status Foo::getStatus() 
{
    return status;
}

that was used in
f.getStatus().setConnectionStatus(ConnectionStatus::Connected);

Since Foo had setStatus(), one could have used it:

auto s = f.getStatus();
s.setConnectionStatus(ConnectionStatus::Connected);
f.setStatus(s);

which is not pretty.

It is the Foo hat has private members (regardless of what those members are), so logically it is the Foo that should provide interface -- "setters", etc -- for them:

class Foo
{
public: 
	void setConnectionStatus(ConnectionStatus val)
	{
		status.setConnectionStatus(val);
	}
...

whether that is any prettier or not is one thing.

Another consideration, usually discussed with pimpl idiom, is what Foo reveals of itself?
The user of Foo must obviously know Foo, and ConnectionStatus, but does it have to know about class Status?
(Furthemore, if the Status is private, then it could be a struct that does not need getters and setters of its own.)

[ElusiveTau]

coder777 wrote:

Don't make your life harder than necessary. When you have both setter and getter your member variable is basically public just with overhead.
So I suggest to remove the getter/setter overhead and make the member public.

jonnin wrote:

I realize this code may be just an example to demonstrate for a question and study purposes. But if you are crafting a program with this kind of monstrosity in it, I beg you to reconsider.

I 100% agree - it is equivalent to making the underlying member public. I'd never use a getter/setter unless reading from or writing to the member had some side effect (validation), or difficult to do without introducing underlying logic e.g., the member's value comes from a database or needs to be computed.

[ElusiveTau]

kigar64551 wrote:

In most cases, a "getter" function should not return a non-const reference, because leaking a non-const references to "internal" data is usually undesired. It would allow the caller to modify "internal" data (via the reference) without using the "setter" function!

Returning a const reference from a "getter" function can make sense, e.g. if the data to be return is big and you don't want to return a copy. But, in this case, you are just returning a simple enum value (eseentially an int), so returning a reference doesn't make much sense.

In cases where you do return a non-const reference from your "getter" function – typically a reference to some sort of "nested" object that you explicitly do want the caller to be able to modify – a separate "setter" function is not needed and wouldn't make much sense!

I have to follow a guideline to make all data private. We can see the flaw of following this guideline blindly without considering how bloated code can get when use case calls for a public data member.

[kigar64551]

I have to follow a guideline to make all data private.

This is not unusual. However, as pointed out before, you have to be very careful to not leak a mutable (non-const) reference or pointer to the "internal" (private) data via your "getter" functions (or in any other way), because the caller could use the mutable reference to modify the "internal" data without using your "setter" function – which contradicts the reason to have a "setter" function.

The exception is when your class contains some sort of "nested" object and you explicitly want the caller to be able to access that "inner" object with the ability to modify it. In that case, you can make the "getter" function return a mutable (non-const) reference to the "inner" object. But then no corresponding "setter" function is needed, in the "outer" object, because callers can modify the "inner" object already! In the above example, this is the case with Foo::getStatus(), which returns a reference to the inner ConnectionStatus instance.

https://cplusplus.com/forum/beginner/285960/#msg1244218

[keskiverto]

The 'status' is clearly a "nested" object of Foo. Does the "all data private" allow access to nested objects, or does it requires the Foo to have interface for them?

E.g. which of these keeps the data more private:

Foo foo;
foo.getStatus().setConnectionStatus(ConnectionStatus::Connected); // #1
foo.setConnectionStatus(ConnectionStatus::Connected);             // #2 

[TheIdeasMan]

My view with regard to interface and not needing setters and maybe needing getters :

1. When the interface functions do all that is required for the private members;
2. When state is set upon construction and never changed after that. Would need getter if interface doesn't do it.

Option 2 doesn't apply here; the values may change often.

Duthomhas code does that: it has a Connect, Disconnect interface.

With validation, if the values are changing a lot, I think it is worth it to have a Validation class with a protected function for each member variable that validates it. These functions can be called from the constructor, and again from whichever interface function changes a value. So it saves writing identical code twice.

One of the main arguments about encapsulation is that one has a function, so that it can altered at a later time. When there is only 1 value in a class, one could make use of the operator() function. Use operator()() to return the value (the getter ) and operator()(/*some arguments*/) to set the value. But I would only do that if there was some reason not to use the interface, such as with strong types.

Also, this is a connection to a resource, so should it be using RAII - acquire connection in constructor, release it in destructor? May also require a mutex?