Code review

Elysia · 05-09-2008, 04:42 PM

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

Daved · 05-09-2008, 05:24 PM

What is the purpose of this class? What are the intended uses?

cpjust · 05-09-2008, 05:54 PM

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.

iMalc · 05-09-2008, 06:43 PM

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 ?

cpjust · 05-09-2008, 09:15 PM

Quote:

Originally Posted by iMalc

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

Elysia · 05-10-2008, 01:39 AM

Quote:

Originally Posted by Daved

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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.

Quote:

Originally Posted by cpjust

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

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?

anon · 05-10-2008, 02:52 AM

Quote:

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.

Elysia · 05-10-2008, 03:05 AM

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.

Elysia · 05-10-2008, 03:10 AM

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.

manav · 05-10-2008, 03:11 AM

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

Elysia · 05-10-2008, 03:12 AM

You can do so yourself.
Edit -> delete.

manav · 05-10-2008, 03:15 AM

Quote:

Originally Posted by Elysia

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 . . .

Elysia · 05-10-2008, 03:16 AM

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.

manav · 05-10-2008, 03:21 AM

Quote:

Originally Posted by Elysia

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?

Elysia · 05-10-2008, 03:23 AM

Sure, it's what I do. But classes are better than free functions.

05-09-2008, 05:24 PM	#2
Daved Registered User Join Date: Jan 2005 Posts: 7,137	What is the purpose of this class? What are the intended uses?
Daved is offline

05-09-2008, 05:54 PM	#3
cpjust and the hat of sweating Join Date: Aug 2007 Location: Toronto, ON Posts: 3,120	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.
cpjust is offline

05-09-2008, 06:43 PM	#4
iMalc Algorithm Dissector Join Date: Dec 2005 Location: New Zealand Posts: 2,476	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 ? __________________ My homepage Advice: Take only as directed - If symptoms persist, please see your debugger Last edited by iMalc; 05-09-2008 at 06:54 PM.
iMalc is offline

05-10-2008, 03:11 AM	#10
manav 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
manav is offline

Thread Tools
Show Printable Version Email this Page
Display Modes
Linear Mode Switch to Hybrid Mode Switch to Threaded Mode

Similar Threads
Thread	Thread Starter	Forum	Replies	Last Post
Code Review for upcoming contest	Darryl	Contests Board	2	02-28-2006 02:39 PM
Problem : Threads WILL NOT DIE!!	hanhao	C++ Programming	2	04-16-2004 01:37 PM
True ASM vs. Fake ASM ????	DavidP	A Brief History of Cprogramming.com	7	04-02-2003 04:28 AM
Interface Question	smog890	C Programming	11	06-03-2002 05:06 PM
Who will map the scan code (inserted by VKD_Force_keys) to virtual key code?	Unregistered	Windows Programming	0	02-21-2002 06:05 PM