Opinion on this paradigm please
-
Hi
I have just started working on a new project and have come across this class and I dont really like the paradigm@@ used to implement the getters and setters.
Has anyone seen this method before?
Is it good?
is it bad?Header File:
@
class Tool
{
public:
Tool();
virtual ~Tool();bool setName(const std::string &name); bool setMechanicalSerialNumber(const std::string &serialNumber); bool getName(std::string &name) const; bool getMechanicalSerialNumber(std::string &serialNumber) const; void clearName(); void clearMechanicalSerialNumber(); virtual void clearAll() = 0;protected:
private:
std::string m_name;
bool m_nameSet;std::string m_mechanicalSerialNumber; bool m_mechanicalSerialNumberSet;};
@cpp file:
@
#include "Tool.h"Tool::Tool()
: m_name("")
, m_nameSet(false)
, m_mechanicalSerialNumber("")
, m_mechanicalSerialNumberSet(false)
{
}Tool::~Tool()
{
}bool Tool::setName(const std::string &name)
{
m_name = name;
m_nameSet = true;
return (m_nameSet);
}
bool Tool::setMechanicalSerialNumber(const std::string &serialNumber)
{
m_mechanicalSerialNumber = serialNumber;
m_mechanicalSerialNumberSet = true;
return (m_mechanicalSerialNumberSet);
}bool Tool::getName(std::string &name) const
{
bool success = false;
if (m_nameSet)
{
name = m_name;
success = true;
}
return (success);
}
bool Tool::getMechanicalSerialNumber(std::string &serialNumber) const
{
bool success = false;
if (m_mechanicalSerialNumberSet)
{
serialNumber = m_mechanicalSerialNumber;
success = true;
}
return (success);
}void Tool::clearName()
{
m_name = "";
m_nameSet = false;
}
void Tool::clearMechanicalSerialNumber()
{
m_mechanicalSerialNumber = "";
m_mechanicalSerialNumberSet = false;
}void Tool::clearAll()
{
clearName();
clearMechanicalSerialNumber();
}@
-
Ke? I have not, but what does this have to do with this code? :)
In my opinion, this is ugly and flawed code, but others may differ. A string is either empty or not. I see no point in the boolean variables. Also, a getter returning by parameter reference... doh.
-
Hi, I have seen/used some variants of that paradigm when set/getting the data was done by putting/retrieving information from a device/remote service/etc... that could fail.
But not for simple "container" classes like this, i'd rather test the returned value for null/empty/invalid value (Qt style).
if setting a string fails then there is a much bigger problem at hand, the same applies for retrieving it.
I also don't see (in this case) why is clearAll pure virtual, nothing else in the class is virtual so it doesn't make it a good candidate for inheritance. Anyone wanting to use it would have to inherit it, implement clearAll() by simply calling the default implementation. Not very good IMHO
Just my 2 cents
-
I've had discussions in the past about such an approach, in most cases it was made by people that learned the trade with C, not C++.
People have told me often that they want to return stuff though an argument-ref because there may be more than one return variable.
In all cases so far I've seen that the multiple arguments really belong together and should at minimum be in a struct, or even in a dedicated class.I've come to see the 'output argument' concept as an anti-pattern, where users of this apprach most likely need to create new classes to hold data instead of passing around individual parts that really belong together (like a string and its length).
Back to your example;
returning a bool to check if things went Ok when 99.999% of the time it will go Ok is not the best approach. I refer to Qts QString::toInt(*bool = 0); for a much better approach to solve this issue. -
Avoid argument-ref wherever possible, for the simple reason that it almost always leads to bloated and somewhat ugly code.
Just imagine you want to std::cout the name and the serial number:
@
void printArgumentRef(const Tool &tool)
{
QString name;
if (tool.getName(name) == false)
...QString mechanicalSerialNumber; if (tool.getMechanicalSerialNumber(nechanicalSerialNumber) == false) ... std::cout << name << mechanicalSerialNumber;}
void printReturn(const Tool &tool)
{
std::cout << tool.getName() << tool.getMechanicalSerialNumber();
}
@
(This is also one of the major drawbacks of streaming operators, as for example in QDataStream).Keep in mind that there is no longer a technical reason to use argument-ref. Any modern compiler has return-value optimization which eliminates the cost of the temporary and C++11 has move semantics built-in.
In addition, I tend to agree that the state of data is part of the data itself (and should not, for instance, be split into a return value and a parameter as in your case). You'll find this paradigm in Qt all over as well someObject.isValid()). It is the code using the value beeing responsible for validation (std::cout.operator<<), not generating or simply passing the value (printArgumentRef()).
boost::optional has already been mentioned. I want to add expected<T>, as introduced by Andrei Alexandrescu and presented on his talk on "Systematic Error Handling in C++":http://channel9.msdn.com/Shows/Going+Deep/C-and-Beyond-2012-Andrei-Alexandrescu-Systematic-Error-Handling-in-C at the C+B2012.
-
In Haskell, theres a standard Maybe type, defined as
@data Maybe a = Just a | Nothing@Function
@isJust :: Maybe a -> Bool@returns True, when parameter is not Nothing.
@fromJust :: Maybe a -> a@extracts the value from parameter when it's a Just.
There can be numerous implementations of Maybe monad in C-family. Some libraries favour returning bool/int, where return value is the corresponding value of isJust; others return int, where 0 returned means everything's okay, and any other value suggests an error occured. Incapsulation of value and flag is more prefered. boost::optional has been mentioned above, I believe it's the same as Maybe, while any automatic pointer could do the trick as well, where NULL means Nothing and not NULL means Just something. In Qt, there's a QScopedPointer, in STL there's a std::shared_ptr. Default-constructed value would be Nothing.
