Thread: Code review

Originally Posted by iMalc

In the destructor, don't bother to set m_nSize to zero. Integers don't require destruction - it's excess baggage.

Well, just wanted to be safe and and all that, so you know, setting it to 0 makes sense, but ah I guess it doesn't matter. I don't know if the compiler can optimize it either...

Your copy-constructor is very contrived and inefficnent...

Well, I was pursuing a re-use code design, so it forwards call deeper into the class as a result.

Starting from that call to PrepareBuffer is where it gets really nasty. Why would the nNeededSize you pass in ever care about the current length? Why is the new size allocated the length of the right hand string*2 plus 2 plus the default size*2? That's pretty excessive!

I guess, but it's an easy algorithm to make sure I have enough space for the buffer and add some more so we don't reallocate all the time.

Then you do the unthinkable and call the destructor directly. Sure you're allowed to do that, but Ewwww! The only useful thing the destructor does is delete [] m_pData so why not just do that?

Reusing code and well... the destructor is supposed to delete/get rid of the buffer, so calling that would take care of that. Save duplicating code was my motive.

I can't work out why Assign would call Empty then operator +=.

Basically, code reuse. Truncate the current buffer and then call += which does the real job. And since there's no data, it appends to the start.

It does not make sense for the 1 parameter version of GetLength, GetFormatFunction, GetVFormatFunction, GetVFormatCountFunction, and GetCompareFunction to be member template functions. You've totally mixed up how to do that. What you want is for example with GetCompareFunction, a default template that provides no implementation. Then you provide specialisations with the template<> syntax where instead of using the type in any way at all you use char and wchar_t.
http://www.cprogramming.com/tutorial...alization.html
http://www.cplusplus.com/doc/tutorial/templates.html

In other words, just call specialized templates to do the actual code instead of acquiring a function pointer?

I'm glad you've done the two-pass approach for your Replace function.

Yeah, I remembered MFC did the same and figured it would be better to make sure there was appropriate space so that I didn't have to reallocate each time I had to copy.

Originally Posted by Elysia

Well, just wanted to be safe and and all that, so you know, setting it to 0 makes sense, but ah I guess it doesn't matter. I don't know if the compiler can optimize it either...

I'm not sure if the compiler would either, but if it can then I would.

Well, I was pursuing a re-use code design, so it forwards call deeper into the class as a result.

I guess, but it's an easy algorithm to make sure I have enough space for the buffer and add some more so we don't reallocate all the time.

Reusing code and well... the destructor is supposed to delete/get rid of the buffer, so calling that would take care of that. Save duplicating code was my motive.

Code reuse is a noble goal, but copy-construction deserves to be a special case. It doesn't have to deal with any of the compexities that arrise in the other situations. Just as your copy-assignment is done as a special case, I hope you'd reconsider using the far simpler special case code for copy-construction. There's a point at which it's better to write special case code instead of continually building upon reuse upon reuse, and I'd say this is definitely it.

Basically, code reuse. Truncate the current buffer and then call += which does the real job. And since there's no data, it appends to the start.

Something should really be implemented in terms of other things that are logically less work, not things that are more work. That means not implementing = in terms of +=, since += is logically more work. Otherwise you'd find that users of this class that thought they were optimising their code would wonder why it got slower. If it was changed around to be the other way then it should be about the same amount of code.

In other words, just call specialized templates to do the actual code instead of acquiring a function pointer?

Yeah that's it.

Yeah, I remembered MFC did the same and figured it would be better to make sure there was appropriate space so that I didn't have to reallocate each time I had to copy.

So does the ATL one.

There is a lot to be learned from attempting to reinvent the wheel.
By coming across the same issues as others who made the widely-used implementations did, you're not just learning how to write a string class, but you're learning a new appreciation for why some of it is done the way it is, as well as how to use it more effectively, having gained an understanding of how it works "under the hood".

I don't think that most people write their own and continue to use it for a very long time. As people's experience increases they learn new things like "constructor initialisation lists", "templates", "const-correctness", "OS-specific optimisations", "exception guarantees", "throw specifications", "traits classes" etc and each one of these new things entails substantial changes to their own class. Soon enough they learn one more thing about writing a better X that they just can't be bothered doing yet another rewrite for, and eventually most end up using the ones provided with the compiler that do all of the aforementioned and more. Either that or they become experts who end up writing the libraries for compilers

How long it takes to get to that point depends on the individual, but I think Elysia is learning quickly.

Originally Posted by Daved

That doesn't really answer question. It's hard to critique something if we don't know the goals behind it. What is the expected result?

My expectations are a fully OOP class that I can use instead of CString. It will suck up any over-the-years stand-alone utility functions and integrate them into its core.
Also, of course, it will give some learning experience, seeing as it already has given me trouble!
And another point... this allows me to get rid of one MFC dependency. Which is always a good thing in a code library where this code will belong.

The only reasonable answer I can come up with is that you are doing it as a learning exercise, and the expected result is that you will have gained experience when you are done.

Well, I do like re-inventing wheels, mostly because I do it all in free time, but also because it will together with other pieces make a new framework that allows for an easy and powerful OOP framework that I will be using within my projects.
Experience is a bonus plus.

It sounds, though, that you just don't like non-member functions for some reason and have decided to implement any string related function that you like to use in a new class. Is that accurate? You hope to have a better string class when you are done (even though it seems quite likely that you won't- primarily because whatever you end up with will be something that only you know).

Yes, this does sound about accurate. It allows for an OOP class.

If you are truly trying to reinvent the wheel in the shape you like I wish you luck.

Thanks. This is not the only wheel I've reinvented so it's nothing new XD

I hacked away at the source and made some progress.
Added Left, added specialized std::swap, added < and > operators, added custom allocator class and added iterator support, as well. Also revamped that copy constructor.
Here's my updated source.

But I'm still having problems with the free operators. Some of them aren't recognized it seems.
If I try to assign a CString object, I get an error saying no operator could handle it. Which means that the compiler isn't considering the operator = (const T*) for some reason. There's probably more broken operators, as well, because it only mentioned the integral operators as suggestions!

Help would be appreciated on that.