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();
    }

    @


  • Moderators

    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.


  • Lifetime Qt Champion

    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.



  • Thinking about it more, your example is also a C-style separation of data that really should be grouped in a class.
    If you introduce a struct that holds a std::string and a bool you can simplify the class enormously.



  • That would be something like boost::optional



  • 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.



  • Thanks to everyone
    Lots to think about



  • 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.


Log in to reply
 

Looks like your connection to Qt Forum was lost, please wait while we try to reconnect.