CODEWITHSUNDEEP
c++templatesconstantsgetter
Vincent
Vincent

Reputation: 60441

Secure direct access to class members

I wonder if the following syntax can be "admitted" or if good practices consider this as coming from hell. The goal would be to add a level of protection to force the developer to be well-conscious of what he is doing. Here is the syntax :

class MyClass
{
    public:
        template<bool RemoveProtection = false> 
        inline std::ofstream& writeStream()
        {
            static_assert(RemoveProtection, "You're doing it wrong");
            return _writeStream;
        }

        inline const std::ofstream& writeStream() const
        {
            return _writeStream;
        }

    protected:
        std::ofstream _writeStream;
};

The use would be :

x.writeStream().good(); // <- OK
x.writeStream().put('c'); // <- NOT OK
x.writeStream<true>().put('c'); // <- OK

I find this as a convenient way to tell to the developer : "Beware, you are using a low-level function and you have to be careful with what you are doing". Is it a "acceptable" way of providing a direct access to class members with a kind of "protection" ? Is there other way of coding a such thing ?

Upvotes: 0

Views: 111

Answers (1)

Zeta
Zeta

Reputation: 105905

Have a look at meagar's comment:

You're making your code ugly, harder to maintain and inconvenient for... what exactly? Define your interface. That is the interface to your class. Do not allow developers to bypass it by using some ridiculous template flag hackery. If you're writing code, you always have to know what you're doing. Having to explicitly type <true> to indicate you especially know what you're doing is just... very, very wrong-headed. Developers have documentation. They don't need training wheels and artificial restrictions, they need clear concise code that lets them get things done. – meagar 2012-10-06 02:41:53Z

A class you provide to others should never be able to get into an unpredicted state when another user uses it. An unpredicted state is in this case a state which you never considered when you were writing the class. As such you should either never allow access to low-level methods of your class or document possible flaws.

Lets say you're writing a logger:

struct MyLogger{
    MyLogger(std::string filename) : stream(filename.c_str()){}      
    template <typename T>
    MyLogger& operator<<(const T& v){ stream << v << " "; return *this;}

private:
    std::ofstream stream;
};

Ignore that there isn't a copy constructor and that the assignment operand is missing. Also ignore that it's a crude logger, it doesn't even provide a timestamp. However, as you can see, the state of the logger depends completely on the logger's methods, e.g. if the file has been successfully opened it won't be closed until the logger gets destroyed.

Now say that we use your approach:

struct MyLogger{
    MyLogger(std::string filename) : stream(filename.c_str()){}
    template <typename T>
    MyLogger& operator<<(const T& v){ stream << v << " "; return *this;}

    template<bool RemoveProtection = false> 
    inline std::ofstream& writeStream()
    {
        static_assert(RemoveProtection, "You're doing it wrong");
        return stream;
    }

    inline const std::ofstream& writeStream() const
    {
        return stream;
    }

private:
    std::ofstream stream;
};

And now someone uses the following code

logger.writeStream<true>.close();

Bang. Your logger is broken. Of course it's the users fault, since they used <true>, didn't they? But more often than not a user is going to copy example code, especially if he uses a library for the first time. The user sees your example

logger.writeStream().good(); // <- OK
logger.writeStream().put('c'); // <- NOT OK
logger.writeStream<true>().put('c'); // <- OK

and ignores the documentation completely at first. Then he's going to use the first and the last version. Later he discovers that the last version works every time! What a miraculous thing <true> is. And then he starts to blame you for evil things that happen, so you have protect yourself from blatant flames with a documentation which includes a warning:

    /**
    * \brief Returns a reference to the internal write stream
    *
    * \note You have to use the template parameter `<true>` in order to use it
    *
    * \warning Please note that this will return a reference to the internal 
    *          write stream. As such you shouldn't create any state in which 
    *          the logger cannot work, such as closing the stream.        
    */
    template<bool RemoveProtection = false> 
    inline std::ofstream& writeStream()
    {
        static_assert(RemoveProtection, "You're doing it wrong");
        return stream;
    }

So, what did we get? We still had to put that warning somewhere. It would have been much simpler if we made stream public:

struct MyLogger{
    MyLogger(std::string filename) : stream(filename.c_str()){}
    template <typename T>
    MyLogger& operator<<(const T& v){ stream << v << " "; return *this;}

    /**
    * The internal write stream. Please look out that you don't create
    * any state in which the logger cannot work, such as closing the stream.
    */
    std::ofstream stream;
};

or sticked to

/** >put warning here< */
inline std::ofstream & writeStream()
{
    return stream;
}

Woops. So either don't allow access to your low-level methods (build a wrapper to specific std::ofstream methods if they should be allowed to use them) or document the possible flaws which can happen if you alter the object's internals to much, but don't go the middle-way and make it look okay with a static_assert.

Upvotes: 2

Related Questions