Excercise in class design - code review

Thread Solved
Reply

Join Date: Apr 2008
Posts: 35
Reputation: tymk is an unknown quantity at this point 
Solved Threads: 7
tymk tymk is offline Offline
Light Poster

Excercise in class design - code review

 
0
  #1
Dec 27th, 2008
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!

  1. #ifndef _UTILITY_NAMEVALUEPAIR_H_
  2. #define _UTILITY_NAMEVALUEPAIR_H_
  3.  
  4. #include <sstream>
  5. using std::ostringstream;
  6.  
  7. #include <string>
  8. using std::string;
  9.  
  10. namespace Utility
  11. {
  12. template<typename T>
  13. class NameValuePair
  14. {
  15. public:
  16. static const float Version;
  17.  
  18. NameValuePair(const string&, const T&);
  19. NameValuePair(const NameValuePair<T>&);
  20. virtual ~NameValuePair();
  21.  
  22. virtual const NameValuePair<T>& operator =(const NameValuePair<T>&);
  23. virtual bool operator ==(const NameValuePair<T>&) const;
  24. virtual bool operator !=(const NameValuePair<T>&) const;
  25.  
  26. virtual const string& getName() const;
  27. virtual void setName(const string&);
  28.  
  29. virtual const T& getValue() const;
  30. virtual void setValue(const T&);
  31.  
  32. virtual const string ToString() const;
  33.  
  34. private:
  35. string _name;
  36. T _value;
  37.  
  38. NameValuePair(){};
  39. };
  40.  
  41. // Version
  42. template <typename T>
  43. const float NameValuePair<T>::Version = 0.1f;
  44.  
  45. // Life Cycle
  46. template <typename T>
  47. NameValuePair<T>::NameValuePair(const string& name, const T& value)
  48. {
  49. _name = name;
  50. _value = value;
  51. }
  52.  
  53. template <typename T>
  54. NameValuePair<T>::NameValuePair(const NameValuePair<T>& pair)
  55. {
  56. _name = pair._name;
  57. _value = pair._value;
  58. }
  59.  
  60. template <typename T>
  61. NameValuePair<T>::~NameValuePair()
  62. {
  63. }
  64.  
  65. // Operators
  66. template <typename T>
  67. const NameValuePair<T>& NameValuePair<T>::operator =(const NameValuePair<T>& rhs)
  68. {
  69. if (rhs != *this)
  70. {
  71. _name = rhs._name;
  72. _value = rhs._value;
  73. }
  74. return *this;
  75. }
  76.  
  77. template <typename T>
  78. bool NameValuePair<T>::operator ==(const NameValuePair<T>& rhs) const
  79. {
  80. return ((_name == rhs._name) && (_value == rhs._value));
  81. }
  82.  
  83. template <typename T>
  84. bool NameValuePair<T>::operator !=(const NameValuePair<T>& rhs) const
  85. {
  86. return !(*this == rhs);
  87. }
  88.  
  89. // Properties (getter/setters)
  90. // Name
  91. template <typename T>
  92. const string& NameValuePair<T>::getName() const
  93. {
  94. return _name;
  95. }
  96.  
  97. template <typename T>
  98. void NameValuePair<T>::setName(const string& name)
  99. {
  100. _name = name;
  101. }
  102.  
  103. // Value
  104. template <typename T>
  105. const T& NameValuePair<T>::getValue() const
  106. {
  107. return _value;
  108. }
  109.  
  110. template <typename T>
  111. void NameValuePair<T>::setValue(const T& value)
  112. {
  113. _value = value;
  114. }
  115.  
  116. // Public Methods
  117. template <typename T>
  118. const string NameValuePair<T>::ToString() const
  119. {
  120. ostringstream stringBuilder;
  121. stringBuilder << _name << "=" << _value;
  122. string returnString = stringBuilder.str();
  123. return returnString;
  124. }
  125. }
  126.  
  127. #endif
Reply With Quote Quick reply to this message  
Join Date: Oct 2008
Posts: 1,839
Reputation: ddanbe has much to be proud of ddanbe has much to be proud of ddanbe has much to be proud of ddanbe has much to be proud of ddanbe has much to be proud of ddanbe has much to be proud of ddanbe has much to be proud of ddanbe has much to be proud of ddanbe has much to be proud of 
Solved Threads: 266
ddanbe's Avatar
ddanbe ddanbe is online now Online
Posting Virtuoso

Re: Excercise in class design - code review

 
0
  #2
Dec 27th, 2008
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.
Today is a gift, that's why it is called "The Present".
Make love, no war. Cave ab homine unius libri.
Danny
Reply With Quote Quick reply to this message  
Join Date: Apr 2008
Posts: 35
Reputation: tymk is an unknown quantity at this point 
Solved Threads: 7
tymk tymk is offline Offline
Light Poster

Re: Excercise in class design - code review

 
0
  #3
Dec 27th, 2008
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?
Reply With Quote Quick reply to this message  
Join Date: Jul 2008
Posts: 2,001
Reputation: ArkM has much to be proud of ArkM has much to be proud of ArkM has much to be proud of ArkM has much to be proud of ArkM has much to be proud of ArkM has much to be proud of ArkM has much to be proud of ArkM has much to be proud of ArkM has much to be proud of 
Solved Threads: 343
ArkM's Avatar
ArkM ArkM is offline Offline
Postaholic

Re: Excercise in class design - code review

 
0
  #4
Dec 27th, 2008
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 ...
Reply With Quote Quick reply to this message  
Join Date: Apr 2008
Posts: 35
Reputation: tymk is an unknown quantity at this point 
Solved Threads: 7
tymk tymk is offline Offline
Light Poster

Re: Excercise in class design - code review

 
0
  #5
Dec 27th, 2008
Originally Posted by ArkM View Post
Alas, my conclusion is: too many defects.
Thanks, this is exactly the type of feedback I'm looking for.
Originally Posted by ArkM View Post
1. I have never seen float type version number. It's cool!
Really? Is this one of your defects? If so, why?

Originally Posted by ArkM View Post
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.
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.

Originally Posted by ArkM View Post
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).
Understood.

Originally Posted by ArkM View Post
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?
I suppose I omitted it and will add it.

Originally Posted by ArkM View Post
5. I consider system heaser includes and using directives in user .h files as a very bad and dangerous practice.
Not sure how to avoid this in a template class? Especially if following the advice in #6 below? Why dangerous?

Originally Posted by ArkM View Post
6. It's a good practice to place simplest template member function definitions in the class definition or declare them as inline explicitly. Ignored.
Noted.


Originally Posted by ArkM View Post
7. This class has a virtual destructor (considered as a base of) but it has no protected members: unusual and suspicious symptom.
Yes, probably should make these members protected. Suspicious symptom?

Originally Posted by ArkM View Post
8. No comments or rudimentary useless comments.
Noted.

Originally Posted by ArkM View Post
I think it's not a professionally designed class template. I prefer std::pair till now ...
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!
Reply With Quote Quick reply to this message  
Join Date: Jul 2008
Posts: 2,001
Reputation: ArkM has much to be proud of ArkM has much to be proud of ArkM has much to be proud of ArkM has much to be proud of ArkM has much to be proud of ArkM has much to be proud of ArkM has much to be proud of ArkM has much to be proud of ArkM has much to be proud of 
Solved Threads: 343
ArkM's Avatar
ArkM ArkM is offline Offline
Postaholic

Re: Excercise in class design - code review

 
0
  #6
Dec 28th, 2008
#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++.
Reply With Quote Quick reply to this message  
Join Date: Apr 2008
Posts: 35
Reputation: tymk is an unknown quantity at this point 
Solved Threads: 7
tymk tymk is offline Offline
Light Poster

Re: Excercise in class design - code review

 
0
  #7
Dec 29th, 2008
Originally Posted by ArkM View Post
#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".
Thanks, this is much more honest, direct, informative and helpful than "It's cool!"
Originally Posted by ArkM View Post
#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.
OK, now I see what you mean.
Originally Posted by ArkM View Post
#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?
I understand eliminating the using std::string but I don't understand eliminating the #include <string> . Would the preferred practice be a forward declaration of std::string?
Reply With Quote Quick reply to this message  
Join Date: Jul 2008
Posts: 2,001
Reputation: ArkM has much to be proud of ArkM has much to be proud of ArkM has much to be proud of ArkM has much to be proud of ArkM has much to be proud of ArkM has much to be proud of ArkM has much to be proud of ArkM has much to be proud of ArkM has much to be proud of 
Solved Threads: 343
ArkM's Avatar
ArkM ArkM is offline Offline
Postaholic

Re: Excercise in class design - code review

 
0
  #8
Dec 29th, 2008
Originally Posted by tymk View Post
...but I don't understand eliminating the #include <string> . Would the preferred practice be a forward declaration of std::string?
Why not? Let a user includes all needed system headers then includes all third-party .h files then its own .h files... It looks like a honest, simple and logical solution: define system environment then define workbench environment then local environment...

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
Reply With Quote Quick reply to this message  
Join Date: Apr 2008
Posts: 35
Reputation: tymk is an unknown quantity at this point 
Solved Threads: 7
tymk tymk is offline Offline
Light Poster

Re: Excercise in class design - code review

 
0
  #9
Dec 30th, 2008
Ah, ok, I get it now. Thank you for all of your help!
Reply With Quote Quick reply to this message  
Reply

This thread has been marked solved.
Perhaps start a new thread instead?
Message:



Other Threads in the C++ Forum
Thread Tools Search this Thread



About Us | Contact Us | Advertise | DaniWeb | Acceptable Use Policy | RSS Feed

©2003 - 2009 DaniWeb® LLC