Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have implemented a basic object class in C++, and I want to make sure that I have implemented everything efficiently and safely, as I made a few mistakes while making it, which I corrected, but I could have left a few more mistakes behind:

class object
{
private:
    class dummy
    {
    public:
        dummy()
        {
        }
        virtual ~dummy()
        {
        }
        virtual const std::type_info &type() = 0;
        virtual dummy *duplicate() const = 0;
        virtual bool eq(object &) = 0;
    };

    template < typename _Ty > class data : public dummy
    {
    public:
        data()
        {
        }

        data(const _Ty &_Value)
            : __data(_Value)
        {
        }

        ~data()
        {
        }

        const std::type_info &type()
        {
            return typeid(_Ty);
        }

        data *duplicate() const
        {
            return new data<_Ty>(__data);
        }

        bool eq(object &_Obj)
        {
            return _Obj.cast<_Ty>() == __data;
        }

        _Ty __data;
    };

    dummy *d;
public:
    object()
    {
    }

    template < typename _Ty > object(const _Ty &_Value)
        : d(new data<_Ty>(_Value))
    {
    }

    object(const object &_Obj)
        : d(_Obj.d->duplicate())
    {
    }

    ~object()
    {
        if (!empty())
        {
            delete d;
        }
    }

    const std::type_info &type()
    {
        return (empty() ? typeid(void) : d->type());
    }

    object &operator=(object &_Rhs)
    {
        d = _Rhs.d->duplicate();
        return *this;
    }

    object &swap(object &_Rhs)
    {
        std::swap(*this, _Rhs);
        return *this;
    }

    template < typename _Ty > object &operator=(const _Ty &_Value)
    {
        d = new data<_Ty>(_Value);
        return *this;
    }

    template < typename _Ty > _Ty cast()
    {
        if (type() == typeid(_Ty))
        {
            return static_cast<data<_Ty> *>(d)->__data;
        }
        throw std::exception("Invalid cast type");
    }

    bool operator==(object &_Rhs)
    {
        return (type() == _Rhs.d->type() ? d->eq(_Rhs) : false);
    }

    template < typename _Ty > bool operator==(_Ty _Value)
    {
        return (type() == typeid(_Ty) ? cast<_Ty>() == _Value : false);
    }

    bool operator!=(object &_Rhs)
    {
        return !(*this == _Rhs);
    }

    template < typename _Ty > bool operator!=(const _Ty &_Value)
    {
        return !(*this == _Value);
    }

    bool empty()
    {
        return !d;
    }
};
share|improve this question
    
1) Replace { } with = default; 2) Get rid of names starting with underscore(s) –  user2176127 Feb 24 at 21:20
4  
You should stop using reserved identifiers. –  Rapptz Feb 24 at 21:51
    
Your default constructor object(){} will default-initialize the data member dummy *d;. Default-initialization for fundamental types means no initialization is performed. Subsequent calls of empty() might return false. Use value-initialization object() : d() {} or explicit initialization object() : d(nullptr) {}. Also, you don't need to check if(!empty()) before delete d;; deleting a nullptr value is safe. –  dyp Feb 25 at 0:07
    
std::swap(*this, _Rhs); won't do any magic. It will make a three copies (one copy-construction of a "temporary", and two copy-assignments). That's exactly what you want to avoid when supplying a custom swap function. Also, you should consider making it a non-member friend function, see stackoverflow.com/a/5695855/420683 –  dyp Feb 25 at 0:18
3  
Why!!!!!!!!!!!! –  Loki Astari Feb 25 at 7:13
add comment

2 Answers

up vote 4 down vote accepted
object &operator=(object &_Rhs)
{
    d = _Rhs.d->duplicate();
    return *this;
}

I think you're leaking your previous d object.

It's also conventional to verify whether _Rhs and this are the same (and to do nothing if they are), otherwise it will misbehave if you assign an object to itself (like foo = foo;).

share|improve this answer
2  
Or to use copy-and-swap instead. –  Yuushi Feb 25 at 1:26
add comment
  • As per common naming convention, the class names should start with a capital letter.

  • It may be more readable to put the template line above the class line:

    template < typename _Ty >
    class data : public dummy
    {
    }
    
  • Your bool functions should be const. Any member function that doesn't modify data members should be const. Such functions also include accessors ("getters") and the bool operators (operator==, operator!=, operator<, and operator>).

    For instance, with empty():

    bool empty() const
    {
        // ...
    }
    

    and operator==:

    bool operator==(object &_Rhs) const
    {
        // ...
    }
    

    Also, the above parameter should be const& since _Rhs is not being modified.

  • If you're not defining your own constructor and/or destructor, you don't need to make them yourself. The compiler will make them for you.

    However, if you're using C++11, you can have an explicitly defaulted constructor and destructor:

    data() = default;
    ~data() = default;
    
share|improve this answer
2  
A default constructor will be created for you if no other constructor exists. I think it's always clearer to use default for this reason - it shows intent to have a default constructor. –  Yuushi Feb 25 at 1:28
    
@Yuushi: Oh, you mean like data() = default;? If I'm correct, it's for C++11. –  Jamal Feb 25 at 1:43
    
"As per common naming convention, the class names should start with a capital letter." Like the standard library? –  Sebastian Redl Feb 25 at 13:02
    
@SebastianRedl Stroustrup suggests that you capitalize user-defined names in TC++PLv4p156. He then continues, "Note that the language and the standard library use lowercase for types; this can be seen as a hint that they are part of the standard." –  dyp Feb 25 at 21:19
    
@Jamal - the only point I disagree with in your answer is this: "As per common naming convention, the class names should start with a capital letter". In all C++ projects I've worked in, the naming convention was never based on Stroustrup's suggestion; instead, it was either MFC/hungarian convention (yuk!), or lower_case_with_underscores because of uniformity of client code and readability (or similar arguments), or a custom convention (e.g. int mylib_ApiName(const int in_value, int &out_value); or similar). –  utnapistim Mar 3 at 10:50
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.