DaniWeb IT Discussion Community - Excercise in class design

DaniWeb IT Discussion Community (http://www.daniweb.com/forums/index.php)

- C++ (http://www.daniweb.com/forums/forum8.html)

- - Excercise in class design - code review (http://www.daniweb.com/forums/thread164450.html)

Excercise in class design - code review

Hi all. I wanted to run this class by some experienced programmers to see if I have any obvious errors and to get any advice otherwise.
I created a test suite and everything seems to function properly so I'm interested in feedback on overall class design, choice of data types, const correctness, not-so-obvious gotcha!s, etc. Thanks!

#ifndef _UTILITY_NAMEVALUEPAIR_H_

#define _UTILITY_NAMEVALUEPAIR_H_



#include <sstream>

using std::ostringstream;



#include <string>

using std::string;



namespace Utility

{

        template<typename T>

        class NameValuePair

        {

        public:

                static const float Version;



                NameValuePair(const string&, const T&);

                NameValuePair(const NameValuePair<T>&);

                virtual ~NameValuePair();



                virtual const NameValuePair<T>& operator =(const NameValuePair<T>&);

                virtual bool operator ==(const NameValuePair<T>&) const;

                virtual bool operator !=(const NameValuePair<T>&) const;



                virtual const string& getName() const;

                virtual void setName(const string&);



                virtual const T& getValue() const;

                virtual void setValue(const T&);



                virtual const string ToString() const;



        private:

                string _name;

                T _value;



                NameValuePair(){};

        };

        

        // Version

        template <typename T>

        const float NameValuePair<T>::Version = 0.1f;

        

        // Life Cycle

        template <typename T>

        NameValuePair<T>::NameValuePair(const string& name, const T& value)

        {

                _name = name;

                _value = value;

        }

        

        template <typename T>

        NameValuePair<T>::NameValuePair(const NameValuePair<T>& pair)

        {

                _name = pair._name;

                _value = pair._value;

        }

        

        template <typename T>

        NameValuePair<T>::~NameValuePair()

        {

        }

        

        // Operators

        template <typename T>

        const NameValuePair<T>& NameValuePair<T>::operator =(const NameValuePair<T>& rhs)

        {

                if (rhs != *this)

                {

                        _name = rhs._name;

                        _value = rhs._value;

                }

                return *this;

        }

        

        template <typename T>

        bool NameValuePair<T>::operator ==(const NameValuePair<T>& rhs) const

        {

                return ((_name == rhs._name) && (_value == rhs._value));

        }



        template <typename T>

        bool NameValuePair<T>::operator !=(const NameValuePair<T>& rhs) const

        {

                return !(*this == rhs);

        }



        // Properties (getter/setters)

        // Name

        template <typename T>

        const string& NameValuePair<T>::getName() const

        {

                return _name;

        }

        

        template <typename T>

        void NameValuePair<T>::setName(const string& name)

        {

                _name = name;

        }



        // Value

        template <typename T>

        const T& NameValuePair<T>::getValue() const

        {

                return _value;

        }

        

        template <typename T>

        void NameValuePair<T>::setValue(const T& value)

        {

                _value = value;

        }



        // Public Methods

        template <typename T>

        const string NameValuePair<T>::ToString() const

        {

                ostringstream stringBuilder;

                stringBuilder << _name << "=" << _value;

                string returnString = stringBuilder.str();

                return returnString;

        }

}



#endif

Re: Excercise in class design - code review

Nice.
But why let the value be generic and the name not? Or was that the intention in the first place in which case I have said nothing.

Re: Excercise in class design - code review

Thank you. Yes, a non-generic name member was my intent for this excercise. I wanted to start with something simple before I tackle a larger project. One thing I overlooked was error handling. Not sure where it could fit here, any advice?

Re: Excercise in class design - code review

Alas, my conclusion is: too many defects.
1. I have never seen float type version number. It's cool!
2. I don't understand why nobody can declare arrays of NameValuePair (default constructor is private). Everybody can assign arbitrary values to both pair members via setters but it's impossible to define default member values - a very strange project decision.
3. No need in explicit copy constructor and assignment operator definition: they follows by default copy constructor and assignment operator logics (do it member by member).
4. The toString() member function requires

operator<<(string,type)

for a template argument type but template class itself does not redefine its own operator<< for std::string. Why?
5. I consider system heaser includes and using directives in user .h files as a very bad and dangerous practice.
6. It's a good practice to place simplest template member function definitions in the class definition or declare them as inline explicitly. Ignored.
7. This class has a virtual destructor (considered as a base of) but it has no protected members: unusual and suspicious symptom.
8. No comments or rudimentary useless comments.

I think it's not a professionally designed class template. I prefer std::pair till now ;)...

Re: Excercise in class design - code review

Quote: