| | |
Excercise in class design - code review
Thread Solved
![]() |
•
•
Join Date: Apr 2008
Posts: 35
Reputation:
Solved Threads: 7
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!
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!
C++ Syntax (Toggle Plain Text)
#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
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
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
...
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
... •
•
Join Date: Apr 2008
Posts: 35
Reputation:
Solved Threads: 7
Thanks, this is exactly the type of feedback I'm looking for.
Really? Is this one of your defects? If so, why?
I suppose I took the lazy way out. I got as far as thinking I could make objects default to null, but not standard types so I gave up and just made the default constructor private. Any advice regarding this would be appreciated.
Understood.
I suppose I omitted it and will add it.
Not sure how to avoid this in a template class? Especially if following the advice in #6 below? Why dangerous?
Noted.
Yes, probably should make these members protected. Suspicious symptom?
Noted.
I didn't expect it to be professional on my first try, that's why I asked for feedback! Wasn't trying to replace std::pair for you either, just trying to get some practice designing classes the correct way. Thank you for taking a look and sharing your opinion!
Really? Is this one of your defects? If so, why?
•
•
•
•
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.
Noted.
I didn't expect it to be professional on my first try, that's why I asked for feedback! Wasn't trying to replace std::pair for you either, just trying to get some practice designing classes the correct way. Thank you for taking a look and sharing your opinion!
#1. Version 1.2 means "subversion 2 of version 1", not "version 1 + 2/10". The only reliable floating point "is equal" op is "value == 0.0".
#2. You can't declare type arrays if no accessible default constructor for the type. I can't understand why it's so dangerous to declare arrays of pairs.
#5. You open (at least a part of) outside namespace for user module code. That's obviously undesired side effect of such includes. Use qualified names: std::string, std::ostringstream etc if your template has STL dependencies. Document these dependencies. Let the template user provide all system headers for his code.
>Not sure how to avoid this in a template class?
And what's a problem?
Yet another remark. I think such "pair" templates must have two typename parameters for both members. It's so easy to get a proper template specialization for std::string as the key member type.
Conclusion: a pair of unrelated values template is not an interesting generic class because no non-trivial operations for this type. So it's not so good base to study generic programming in C++.
#2. You can't declare type arrays if no accessible default constructor for the type. I can't understand why it's so dangerous to declare arrays of pairs.
#5. You open (at least a part of) outside namespace for user module code. That's obviously undesired side effect of such includes. Use qualified names: std::string, std::ostringstream etc if your template has STL dependencies. Document these dependencies. Let the template user provide all system headers for his code.
>Not sure how to avoid this in a template class?
And what's a problem?
Yet another remark. I think such "pair" templates must have two typename parameters for both members. It's so easy to get a proper template specialization for std::string as the key member type.
Conclusion: a pair of unrelated values template is not an interesting generic class because no non-trivial operations for this type. So it's not so good base to study generic programming in C++.
•
•
Join Date: Apr 2008
Posts: 35
Reputation:
Solved Threads: 7
•
•
•
•
#1. Version 1.2 means "subversion 2 of version 1", not "version 1 + 2/10". The only reliable floating point "is equal" op is "value == 0.0".
•
•
•
•
#2. You can't declare type arrays if no accessible default constructor for the type. I can't understand why it's so dangerous to declare arrays of pairs.
•
•
•
•
#5. You open (at least a part of) outside namespace for user module code. That's obviously undesired side effect of such includes. Use qualified names: std::string, std::ostringstream etc if your template has STL dependencies. Document these dependencies. Let the template user provide all system headers for his code.
>Not sure how to avoid this in a template class?
And what's a problem?
using std::string but I don't understand eliminating the #include <string> . Would the preferred practice be a forward declaration of std::string? •
•
•
•
...but I don't understand eliminating the #include <string> . Would the preferred practice be a forward declaration of std::string? That's why I prefer explicit namespace qualification in all user .h files: let this .h-file user makes decision where to place sacramental (and very dangerous) using namespace std; or avoid it.
It's also a pragmatically authorized practice. For example, it's a good practice in MS VC++ to place ALL needed system headers in the project stdafx.h file (MS VC++ pre-compiled headers container). If so, no need to include system headers in user code at all.
Worst case: your template user gets a simple and clear compiler message about std::string error. OK, he/she inserts
#include <string> before include_template_definition and gets a wonderful value pair class depended on std::string
![]() |
Other Threads in the C++ Forum
- Previous Thread: dllexport for all functions
- Next Thread: need help with prime
| Thread Tools | Search this Thread |
action api array auto based beginner binary bitmap c++ c/c++ calculator challenge char class classes code coding compile console conversion count createcopyofanyfileinc delete deploy desktop developer directshow dll download dynamic dynamiccharacterarray email encryption error file forms fstream function functions game garbage givemetehcodez graph gui hmenu homeworkhelp homeworkhelper iamthwee ifstream input insert int integer java lib linkedlist linker loop looping loops map math matrix memory multiple news node noob output parameter pointer primenumbersinrange problem program programming project python random read recursion reference rpg sockets string strings temperature template test text text-file tree url variable vector video win32 windows winsock wordfrequency wxwidgets






