CodeSOD: Static State

Today’s Anonymous submitter was reviewing some C++ code, and saw this perfectly reasonable looking pattern.

class SomeClass
{
public:
	void setField(int val);
	int getField();
}

Now, we can talk about how overuse of getters and setters is itself an antipattern (especially if they’re trivial- you’ve just made a public variable with extra steps), but it’s not wrong and there are certainly good reasons to be cautious with encapsulation. That said, because this is C++, that getField should really be declared int getField() const– appropriate for any method which doesn’t cause a mutation to a class instance.

Or should it? Let’s look at the implementation.

void SomeClass::setField(int val)
{
	setGetField(true, val);
}

void SomeClass::getField()
{
	return setGetField(false);
}

Wait, what? Why are we passing a boolean to a method called setGet. Why is there a method called setGet? They didn’t go and make a method that both sets and gets, and decide which they’re doing based on a boolean flag, did they?

int SomeClass::setGetField(bool set, int val)
{
	static int s_val = 0;
	if (set)
	{
		s_val = val;
	}
	return s_val;
}

Oh, good, they didn’t just make a function that maybe sets or gets based on a boolean flag. They also made the state within that function a static field. And yes, function level statics are not scoped to an instance, so this is shared across all instances of the class. So it’s not encapsulated at all, and we’ve blundered back into Singletons again, somehow.

Our anonymous submitter had two reactions. Upon seeing this the first time, they wondered: “WTF? This must be some kind of joke. I’m being pranked.”

But then they saw the pattern again. And again. After seeing it fifty times, they wondered: “WTF? Who hired these developers? And can that hiring manager be fired? Out of a cannon? Into the sun?”

Source: Read More