I'm having trouble properly implementing the Proxy pattern. In my project I have an abstract Label class (used as an interface) and its concrete children, such as SimpleLabel and RichLabel. Since every label should have some text, I added a member variable value to the base class. Here's my Label interface:
class Label {
protected:
std::string value;
public:
Label(const std::string&);
virtual std::string getText() const = 0;
virtual ~Label() = default;
};
I need to implement a new type of label - let's call it ProxyLabel - using the Proxy design pattern. The idea is as follows:
Lazy Initialization: The text for the label should be read from standard input only when its getText() method is called for the first time. After the text is read, it should be stored (cached) and automatically returned on subsequent calls.
Timeout Option: After a configurable number of calls to getText(), the user should be given the option to change the label's text or keep using the current one. Conceptually, my ProxyLabel should do something like this:
std::string ProxyLabel::getText() const {
if (!initialised) {
std::cout << "Enter label text: ";
std::getline(std::cin, cache);
// Note: an empty string might be a valid label,
// so we use a separate flag 'initialised' rather than checking if cache is empty.
initialised = true;
requestCount = 1;
return cache;
}
requestCount++;
if (requestCount >= timeout) {
std::cout << "Label text requested " << requestCount
<< " times. Do you want to update label text? (y/n): ";
char response;
std::cin >> response;
std::cin.ignore(); // clear the newline
if (response == 'y' || response == 'Y') {
std::cout << "Enter new label text: ";
std::getline(std::cin, cache);
}
requestCount = 0;
}
return cache;
}
My challenge is that in my base class Label the getText() method is declared as const, which means it cannot modify the object. I would prefer not to change the interface of Label (for example, I don't want to mark the value member as mutable for just one class).
My questions are:
Since I wrote the project in C++ and use classes as interfaces, I added the member variable value in the base class Label (of which SimpleLabel and RichLabel are derived), because all labels will have some text (their content).
Standard practice would suggest that my new class ProxyLabel should inherit from Label but also compose a Label (i.e., hold an internal Label instance).
Question: How do I reconcile this? If ProxyLabel is a child of Label, then in its constructor it must call one of Label's constructors. But Label only has a constructor that takes a std::string (i.e., a value). Doesn't calling this constructor defeat the purpose of lazy initialization - especially if that constructor performs heavy operations? In other words, how can I create a ProxyLabel that delays input without forcing the creation (and potential heavy initialization) of a Label?
My base class method getText() is declared as const, meaning it is not allowed to modify the object. Yet, the proxy must update its internal state (caching the input, updating a counter, and possibly changing the cached text after the timeout).
Question: In this situation, is it acceptable to use mutable member variables in ProxyLabel? I’m hesitant to change the interface for just one class.
when a method is const it is usually safe to call it from multiple threads concurrently. Having mutable members breaks this promise unless you add extra facilities to guarantee this thread-safety.
using mutable members to "lazy initialize" things that should be const is very common, in fact facebook fbstring did something similar for null terminators at one point (it was a pointer so it didn't need to be mutable), but guaranteeing the thread-safety adds a lot more complexity and overhead than you initially anticipated. (in their case this caused performance degradation due to cache line ping-ponging)
you can guarnatee this thread-safety by having only 2 mutable members, one is a mutex, and the other one is a cache with a non-const getText member, that will only be called while holding a lock, this way we know there won't be any race-conditions.
#include <iostream>
#include <string>
#include <mutex>
class Label {
protected:
std::string value;
public:
Label(const std::string& v) : value{ std::move(v) } {}
virtual std::string getText() const = 0;
virtual ~Label() = default;
};
class StringCache
{
public:
std::string getText()
{
if (!initialised) {
std::cout << "Enter label text: ";
std::getline(std::cin, cache);
// Note: an empty string might be a valid label,
// so we use a separate flag 'initialised' rather than checking if cache is empty.
initialised = true;
return cache;
}
return cache;
}
private:
std::string cache;
bool initialised = false;
};
class ProxyLabel : public Label
{
public:
using Label::Label;
std::string getText() const override {
std::lock_guard l{ m };
return s.getText(); // only called while holding a lock
}
private:
mutable StringCache s;
mutable std::mutex m;
};
using mutexes can cause deadlocks, but those deadlocks would've otherwise been crashes, or erroneous output, so always look for a better solution for your current problem, but mutexes are better than having no protection at all, C++ also has std::call_once for lazy initialization, which has less size and performance overhead than a mutex.
your code uses cin to load the string, but in real-life expect a non-global service to provide it, which would be injected through the constructor.
you don't need synchronization if you have a guarantee that the code won't be called from another thread. such as code that interacts with the GUI, but you could also run into recursive calls which would be a bug, marking members as mutable opens a very big can of bugs.
while "lazy initialization" in const methods is sometimes acceptable, having a timeout breaks The Liskov Substitution Principle. i wouldn't recommend you beak it. see the following code
std::unique_ptr<Label> label = getLabel();
auto X = label->getText();
auto Y = label->getText();
anyone looking at Label will think that X == Y , this is not true when your class is used, this will cause bugs, if having a timeout is required then you simply cannot have a proxy, you must either modify the original interface to remove the const or create a new interface for this use-case.
Lastly, i don't think the value member makes any sense, you can just remove it and have each subclass store it as it sees fit, after all Label is an abstract interface, but there are other uses such as the non-virtual interface idiom that can use it.