Code review

This is a discussion on Code review within the C++ Programming forums, part of the General Programming Boards category; Since I'm still a little underexperienced to some of the members here, I thought I'd ask for critics of the ...

  1. #1
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,180

    Code review

    Since I'm still a little underexperienced to some of the members here, I thought I'd ask for critics of the current definition of my string class. It's only the definition, of course, since it would get rather big otherwise and no one wants to browse through a lot of code.

    I may post some snippets of the actual code somewhat later, but anyway, here goes!
    (Note that I haven't replaced the + operators with boost yet, but will do so later since it requires more testing!)

    Code:
    #ifndef String_h__
    #define String_h__
    
    namespace Strings
    {
    	template<typename T> struct StrTraits
    	{
    		static const T* UINT64_T_IDENTIFIER;
    		static const T* INT64_T_IDENTIFIER;
    		static const T* LONG_IDENTIFIER;
    	};
    
    	template<typename T> class CTmplStringBase: public StrTraits<T>
    	{
    	public:
    		CTmplStringBase();
    		CTmplStringBase(const CTmplStringBase& rSrc);
    		CTmplStringBase(const CStringT< T, StrTraitMFC_DLL<T> >& strSrc);
    		CTmplStringBase(const T* strData);
    		CTmplStringBase(const T cData);
    		CTmplStringBase(uint64_t nData);
    		CTmplStringBase(int64_t nData);
    		CTmplStringBase(long nData);
    		~CTmplStringBase();
    		CTmplStringBase& operator = (const T* strData);
    		CTmplStringBase& operator = (const T cData);
    		CTmplStringBase& operator = (uint64_t nData);
    		CTmplStringBase& operator = (int64_t nData);
    		CTmplStringBase& operator = (long nData);
    		CTmplStringBase& operator = (const CTmplStringBase& rSrc);
    		CTmplStringBase& operator = (const CStringT< T, StrTraitMFC_DLL<T> >& rSrc);
    		CTmplStringBase& operator += (const T* strData);
    		CTmplStringBase& operator += (const T cData);
    		CTmplStringBase& operator += (uint64_t nData);
    		CTmplStringBase& operator += (int64_t nData);
    		CTmplStringBase& operator += (long nData);
    		CTmplStringBase operator + (const T* strData) const;
    		CTmplStringBase operator + (const T cData) const;
    		CTmplStringBase operator + (uint64_t nData) const;
    		CTmplStringBase operator + (int64_t nData) const;
    		CTmplStringBase operator + (long nData) const;
    		CTmplStringBase operator + (const CStringT< T, StrTraitMFC_DLL<T> >& strSrc) const;
    		bool operator == (const T* strData) const;
    		bool operator == (const T cData) const;
    		bool operator != (const T* strData) const;
    		bool operator != (const T cData) const;
    		operator const T* () const;
    		void Replace(const T* strReplaceWhat, const T* strReplaceWith);
    		void Replace(const T* strReplaceWhat, const T cReplaceWith);
    		void Replace(const T cReplaceWhat, const T* strReplaceWith);
    		void Replace(const T cReplaceWhat, const T cReplaceWith);
    		CTmplStringBase Right(uint32_t nCount) const;
    		void Delete(uint32_t nStartFrom, uint32_t nCount = 0);
    		uint32_t GetLength() const;
    		T* GetBuffer(uint32_t nMinSize);
    		void ReleaseBuffer();
    		void Truncate(uint32_t nNewSize);
    		int32_t Find(const T* strFind, uint32_t nBeginAt = 0) const;
    		int32_t Find(const T cFind, uint32_t nBeginAt = 0) const;
    		CTmplStringBase Mid(uint32_t nStart, uint32_t nCount = 0) const;
    		void Format(const T* strFormat, ...);
    		void AppendFormat(const T* strFormat, ...);
    		void Empty();
    
    	private:
    		typedef int (fnc_tprintf_s)(T* buffer, size_t sizeOfBuffer, const T* format, ...);
    		typedef int (fnc_tvprintf_s)(T* buffer, size_t sizeOfBuffer, const T* format, va_list argptr);
    		typedef int (fnc_tcscmp)(const T* string1, const T* string2);
    		typedef int (fnc_tvcprintf)(const T* format, va_list argptr);
    
    		void Init();
    		int32_t ReplaceInternal(const T* strReplaceWhat, const T* strReplaceWith, bool bReplace);
    		void FormatInternal(const T* strFormat, va_list arg);
    		uint32_t GetLength(const char* strData) const;
    		uint32_t GetLength(const wchar_t* strData) const;
    		fnc_tprintf_s* GetFormatFunction(const char*) const;
    		fnc_tprintf_s* GetFormatFunction(const wchar_t*) const;
    		fnc_tvprintf_s* GetVFormatFunction(const char*) const;
    		fnc_tvprintf_s* GetVFormatFunction(const wchar_t*) const;
    		fnc_tvcprintf* GetVFormatCountFunction(const char*) const;
    		fnc_tvcprintf* GetVFormatCountFunction(const wchar_t*) const;
    		fnc_tcscmp* GetCompareFunction(const char*) const;
    		fnc_tcscmp* GetCompareFunction(const wchar_t*) const;
    		void CopyStr(const T* strData);
    		void AppendStr(const T* strData);
    		void CopyStrLowLevel(T* strDst, uint32_t nDstSize, const T* strSrc, T** pUnusedPtr) const;
    		void PrepareBuffer(uint32_t nNeededSize);
    		template<typename Type> CTmplStringBase& Assign(Type& Data);
    		template<typename Type> CTmplStringBase& AppendNum(Type& Data, const T* strFormat);
    		template<typename Type> CTmplStringBase Append(Type& Data) const;
    		T* m_pData;
    		T* m_pUnusedStart;
    		uint32_t m_nSize; // Number of elements in m_pData
    		static const uint32_t nDefaultSize = 10;
    	};
    
    #ifndef _AFXEXT
    	template class __declspec(dllimport) CTmplStringBase<wchar_t>;
    	template class __declspec(dllimport) CTmplStringBase<char>;
    #endif
    	typedef CTmplStringBase<char> CStringExA;
    	typedef CTmplStringBase<wchar_t> CStringExW;
    	typedef CTmplStringBase<TCHAR> CStringEx;
    }
    
    #endif // String_h__
    Suggestions and critique most welcome :O
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.
    For information on how to enable C++11 on your compiler, look here.
    よく聞くがいい!私は天才だからね! ^_^

  2. #2
    Registered User
    Join Date
    Jan 2005
    Posts
    7,317
    What is the purpose of this class? What are the intended uses?

  3. #3
    and the hat of sweating
    Join Date
    Aug 2007
    Location
    Toronto, ON
    Posts
    3,545
    Well right off the bat, it's not portable (unless you provide your own CString implementation for non-Windows, or even non-MSVC platforms).
    See: Chapter 63. Use sufficiently portable types in a module's interface

    It's filled with a lot of member functions that could easily be broken out into stand-alone functions. std::string also suffers a bit from that type of code bloat.
    See: Chapter 33. Prefer minimal classes to monolithic classes
    & Chapter 44. Prefer writing nonmember nonfriend functions

    These functions should definitely be moved out of the class:
    Code:
    bool operator == (const T* strData) const;
    bool operator == (const T cData) const;
    bool operator != (const T* strData) const;
    bool operator != (const T cData) const;
    Otherwise you can only compare a (CStringEx == const char*) but not (const char* == CStringEx)...

    Code:
    #ifndef String_h__
    I'd come up with a longer name than that, since there's probably a lot of code that would use that same name. Appending the current date should make it unique enough.

    I'm not exactly sure what your StrTraits struct does? It has 3 static variables all of the same type but different names.

    I like to put documentation with the definition (and document every function, every typedef, every variable...)

    Your constructors & operators support CStrings, but not std::string. That seems like discrimination to me!

    You might also want to allow people to specify their own allocator class (just like the STL containers do).

    Code:
    operator const T* () const;
    I know you like these kinds of conversion operators, but they really are quite evil.
    See: Chapter 40. Avoid providing implicit conversions

    You might want to add iterator support also.

  4. #4
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,266
    I agree with all of what cpjust has mentioned already.

    Of the below two statements, why should only the first one be valid?:
    Code:
    CTmplStringBase x("ABC");
    
    CTmplStringBase y = x + 123;
    CTmplStringBase y = 123 + x;
    In other words, don't declare non-modifying binary operators as member functions. Use the two parameter form and then both sides are implicitly convertable. However the idea of constructing from, or using operator + with integral types here is shakey ground anyway.

    Does Replace replace all occurrences, or just the first one?
    If Replace took a const CTmplStringBase& then you would only need one overload of it.

    No operator <, so it can't easily be used in a std::set, or the key of a std::map

    It could do with a specialisation of std::swap.

    Right and Mid but no Left?

    What would you use m_pUnusedStart for? Isn't it just equal to m_pData+m_nSize ?
    Last edited by iMalc; 05-09-2008 at 06:54 PM.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  5. #5
    and the hat of sweating
    Join Date
    Aug 2007
    Location
    Toronto, ON
    Posts
    3,545
    Quote Originally Posted by iMalc View Post
    However the idea of constructing from, or using operator + with integral types here is shakey ground anyway.
    Yes, that's one thing I forgot to mention.
    Chapter 26. Preserve natural semantics for overloaded operators
    Chapter 27. Prefer the canonical forms of arithmetic and assignment operators

  6. #6
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,180
    Quote Originally Posted by Daved View Post
    What is the purpose of this class? What are the intended uses?
    It's a simple string class that is intended to replace MFC's CString and STL's string. Since MFC wanted to keep data in its class private, there was no way in hell I could derive from it, so I decided one day I'd make my own.

    Quote Originally Posted by cpjust View Post
    Well right off the bat, it's not portable (unless you provide your own CString implementation for non-Windows, or even non-MSVC platforms).
    See: Chapter 63. Use sufficiently portable types in a module's interface
    True. In the beginning, I wasn't going to overload CString operators, but unfortunately, due to ambiguous members, I have to. CString overloads const T*, so it can either can either call operator = (T*) or call the CStringEx constructor for const T* and then operator = (CStringEx&).
    I can't help it. Some of my code relies of on CString and I need to be able to use it with my string class.

    Quote Originally Posted by cpjust View Post
    It's filled with a lot of member functions that could easily be broken out into stand-alone functions. std::string also suffers a bit from that type of code bloat.
    See: Chapter 33. Prefer minimal classes to monolithic classes
    & Chapter 44. Prefer writing nonmember nonfriend functions

    These functions should definitely be moved out of the class:
    Code:
    bool operator == (const T* strData) const;
    bool operator == (const T cData) const;
    bool operator != (const T* strData) const;
    bool operator != (const T cData) const;
    Otherwise you can only compare a (CStringEx == const char*) but not (const char* == CStringEx)...
    Aside from not liking that type of comparisons, stand-alone functions are evil in my view plus friend operators cause other problems, as well. I had to overload an operator + for CString because CString had friend operator + operators that cause ambiguity.

    Quote Originally Posted by cpjust View Post
    Code:
    #ifndef String_h__
    I'd come up with a longer name than that, since there's probably a lot of code that would use that same name. Appending the current date should make it unique enough.
    Point taken I'll make it more "unique." It should probably be named something such as StringEx_h anyway!

    Quote Originally Posted by cpjust View Post
    I'm not exactly sure what your StrTraits struct does? It has 3 static variables all of the same type but different names.
    Here's the tricky part. Due to the format functions, I need to separate from ANSI and UNICODE strings.
    I'll attach the whole source later so you can see why it's necessary. I used specialized templates for the StrTraits struct.

    Quote Originally Posted by cpjust View Post
    I like to put documentation with the definition (and document every function, every typedef, every variable...)
    Absolutely! I'll do that once/if I get DoxyGen running and the class is stable enough.

    Quote Originally Posted by cpjust View Post
    Your constructors & operators support CStrings, but not std::string. That seems like discrimination to me!
    Yep, but then again, I don't want to add more operators, since string can just call .c_str() to make it work. Same thing doesn't apply to CString unfortunately. But if anyone has any good suggestions about that, I'm definitely all ears!

    Quote Originally Posted by cpjust View Post
    You might also want to allow people to specify their own allocator class (just like the STL containers do).
    I suppose that wouldn't be hard to implement. Sure, I could do that

    Quote Originally Posted by cpjust View Post
    Code:
    operator const T* () const;
    I know you like these kinds of conversion operators, but they really are quite evil.
    See: Chapter 40. Avoid providing implicit conversions
    As they say, you can't make a class fool proof. I don't think anyone is sane enough to try to subtract the class with another. If they do, then they are out of their mind. This one is safe enough in my opinion.
    MFC's CString has it and I've never encountered problems with it.

    Quote Originally Posted by cpjust View Post
    You might want to add iterator support also.
    You did remind me that I don't have an index operator either...
    I'm not that familiar with iterators either, but it will probably increase operability with STL, so yes, I'm probably going to take a look at it.

    Quote Originally Posted by iMalc View Post
    Of the below two statements, why should only the first one be valid?:
    Code:
    CTmplStringBase x("ABC");
    
    CTmplStringBase y = x + 123;
    CTmplStringBase y = 123 + x;
    In other words, don't declare non-modifying binary operators as member functions. Use the two parameter form and then both sides are implicitly convertable. However the idea of constructing from, or using operator + with integral types here is shakey ground anyway.
    I agree that an integral type should be its own class, so you can call .ToString(), but that's on some grounds I'm not sure if I want to cover. Then again, perhaps implicit conversions may help here, allowing me to get rid of the integral opperator +.
    But friend operators has their own downsides too. They can easily cause ambiguity.

    Quote Originally Posted by iMalc View Post
    Does Replace replace all occurrences, or just the first one?
    If Replace took a const CTmplStringBase& then you would only need one overload of it.
    It replaces all occurances. Thanks for the tip. That works beautifully it seems. Have to try it with CString too, perhaps.

    Quote Originally Posted by iMalc View Post
    No operator <, so it can't easily be used in a std::set, or the key of a std::map
    In other words, implementation such as strcmp - return if the string is "lower", equal or "higher" than compared string?

    Quote Originally Posted by iMalc View Post
    It could do with a specialisation of std::swap.
    That's unfamiliar to me. Perhaps you could elaborate a little?

    Quote Originally Posted by iMalc View Post
    Right and Mid but no Left?
    Right, forgot that one! Only added functionality I needed (or to make it compile), so naturally I didn't add it, but I forgot.
    Of course it should be there.

    Quote Originally Posted by iMalc View Post
    What would you use m_pUnusedStart for? Isn't it just equal to m_pData+m_nSize ?
    m_nSize keeps track of the full size of character buffer in characters.
    m_pData is a pointer to the start of the buffer.
    m_pUnusedStart is a pointer to where the "unused" portion of the buffer begins or in other words, where it should append data.

    More reading! Yay XD

    Here is the current source for interested parties.
    I'll keep working on those suggestions!

    Another thing I'm having trouble with is error checking. Should it throw? Should it return an error? Should it log an error?
    Obviously I'm no particular fan of either one (there's situations where each one is better suited). For a string class, I'm thinking returning an error if anything, or throwing would be the best choices.
    Should it be a good idea to implement both? Toggled by some check or some such?
    Attached Files Attached Files
    Last edited by Elysia; 05-10-2008 at 02:29 AM.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.
    For information on how to enable C++11 on your compiler, look here.
    よく聞くがいい!私は天才だからね! ^_^

  7. #7
    The larch
    Join Date
    May 2006
    Posts
    3,573
    As they say, you can't make a class fool proof. I don't think anyone is sane enough to try to subtract the class with another. If they do, then they are out of their mind. This one is safe enough in my opinion.
    Code:
    string a(...), b(...);
    if (a = b)
    ...
    This doesn't compile. With your conversion operator it would.

    Also, I believe that since you are not overloading const T* == MyString, "aaa" == str; would still compile, except it would compare pointers.

    One is an honest mistake and the other is something that might be expected from your class and not something completely crazy that people should be punished for.
    Last edited by anon; 05-10-2008 at 02:57 AM.
    I might be wrong.

    Thank you, anon. You sure know how to recognize different types of trees from quite a long way away.
    Quoted more than 1000 times (I hope).

  8. #8
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,180
    Code:
    "aaa" == str;
    That's a mistake I can rectify by making operator == friends.
    Otherwise, sorry, but removing the implicit operator will break the operators and I have to overload another operator for CTmplStringBase, plus it probably force me to overload more CString operator due to ambiguity, and it will break a lot of code which depends on the const T* operator.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.
    For information on how to enable C++11 on your compiler, look here.
    よく聞くがいい!私は天才だからね! ^_^

  9. #9
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,180
    Holy crap, not only is that header incredibly long, but you double-posted, as well.
    I'm not looking into making it bigger, fatter and more functionality at the moment. Only a few fixes and making the code better.
    I could implement an eternity of stuff, but I wouldn't use them. Functionality comes as I need it.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.
    For information on how to enable C++11 on your compiler, look here.
    よく聞くがいい!私は天才だからね! ^_^

  10. #10
    Banned
    Join Date
    Nov 2007
    Posts
    678
    DOUBLE POST!!

    Not my fault I think because of the big size it got doubled up! I hope mods will not mind this! Please delete the extra post

  11. #11
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,180
    You can do so yourself.
    Edit -> delete.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.
    For information on how to enable C++11 on your compiler, look here.
    よく聞くがいい!私は天才だからね! ^_^

  12. #12
    Banned
    Join Date
    Nov 2007
    Posts
    678
    Quote Originally Posted by Elysia View Post
    Holy crap, not only is that header incredibly long, but you double-posted, as well.
    I'm not looking into making it bigger, fatter and more functionality at the moment. Only a few fixes and making the code better.
    I could implement an eternity of stuff, but I wouldn't use them. Functionality comes as I need it.
    But you are supposed to create a class that will replace std::string and CString?

    But there are some really nice ideas in that header.
    - Like formatted string based on functionality (arg(...)) similar to printf
    - Split function that splits on char, string and regular expression.
    - And a hell lot of useful stuff . . .

  13. #13
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,180
    Like I mentioned, I can implement an awful lot of ideas, but I will never use them. I'm looking into implementing things I need. Otherwise it would take forever to finish the class.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.
    For information on how to enable C++11 on your compiler, look here.
    よく聞くがいい!私は天才だからね! ^_^

  14. #14
    Banned
    Join Date
    Nov 2007
    Posts
    678
    Quote Originally Posted by Elysia View Post
    Like I mentioned, I can implement an awful lot of ideas, but I will never use them. I'm looking into implementing things I need. Otherwise it would take forever to finish the class.
    Would not it be better, if, you just implement a few free functions for your purpose, and, put them in a namespace, for grouping?

  15. #15
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,180
    Sure, it's what I do. But classes are better than free functions.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.
    For information on how to enable C++11 on your compiler, look here.
    よく聞くがいい!私は天才だからね! ^_^

Page 1 of 5 12345 LastLast
Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Code Review for upcoming contest
    By Darryl in forum Contests Board
    Replies: 2
    Last Post: 02-28-2006, 01:39 PM
  2. Problem : Threads WILL NOT DIE!!
    By hanhao in forum C++ Programming
    Replies: 2
    Last Post: 04-16-2004, 01:37 PM
  3. True ASM vs. Fake ASM ????
    By DavidP in forum A Brief History of Cprogramming.com
    Replies: 7
    Last Post: 04-02-2003, 03:28 AM
  4. Interface Question
    By smog890 in forum C Programming
    Replies: 11
    Last Post: 06-03-2002, 05:06 PM
  5. Replies: 0
    Last Post: 02-21-2002, 05:05 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21