Code review

This is a discussion on Code review within the C++ Programming forums, part of the General Programming Boards category; Originally Posted by iMalc In the destructor, don't bother to set m_nSize to zero. Integers don't require destruction - it's ...

  1. #61
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,173
    Quote Originally Posted by iMalc View Post
    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.
    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. #62
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,264
    Quote Originally Posted by Elysia View Post
    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.
    Last edited by iMalc; 05-10-2008 at 05:45 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"

  3. #63
    Registered User
    Join Date
    Apr 2008
    Posts
    890
    Personally, I'm stunned at the effort you're expending to re-invent an age-old wheel just to add a couple shiny reflectors between the spokes.

  4. #64
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,264
    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.
    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. #65
    Registered User
    Join Date
    Apr 2008
    Posts
    890
    Quote Originally Posted by iMalc View Post
    There is a lot to be learned from attempting to reinvent the wheel.
    Maybe, if you're doing it on your own time as an academic exercise. In the world of professional software development it's anathema.

  6. #66
    Registered User whiteflags's Avatar
    Join Date
    Apr 2006
    Location
    United States
    Posts
    7,527
    Guys I have serious deja vu. I swear I read these posts like days ahead of now.

  7. #67
    and the hat of sweating
    Join Date
    Aug 2007
    Location
    Toronto, ON
    Posts
    3,545
    Quote Originally Posted by Elysia View Post
    Well... I would say yes, seeing as it must have access to internals to assign.
    But even if not, I'd like to understand how it works.
    operator= must have access to the internals, but I was just talking about operator== which only compares. It can just use the accessor functions.
    You might be able to optimize it a bit by making it a member function, but I'd still provide another operator== stand-alone function to do this:
    Code:
    template <typename T>
    bool operator==( const char*  lhs, const CTmplStringBase<T>&  rhs )
    {
        return (rhs == lhs);  // i.e. Reverse the parameters and call the member function for ==
    }
    and the same for operator!=

  8. #68
    Registered User
    Join Date
    Jan 2005
    Posts
    7,317
    >> It's a simple string class that is intended to replace MFC's CString and STL's string.

    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?

    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.

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

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

  9. #69
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,173
    Quote Originally Posted by Daved View Post
    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
    Last edited by Elysia; 05-11-2008 at 03:05 AM.

  10. #70
    Registered User
    Join Date
    May 2008
    Location
    Paris
    Posts
    248
    Thanks. This is no the only wheel I've reinvented so it's nothing new XD
    Haha! I know the problem, it's just no fun to spend your time looking for has already been done. Why did we ever invent harmonization and built-in libraries? How are we now supposed to have fun?

  11. #71
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,173
    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.
    Attached Files Attached Files
    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. #72
    Registered User
    Join Date
    Oct 2001
    Posts
    2,129
    Code:
    template<> const char* StrTraits<char>::UINT64_T_IDENTIFIER = "%I64u";
    template<> const char* StrTraits<char>::INT64_T_IDENTIFIER = "%I64i";
    template<> const char* StrTraits<char>::LONG_IDENTIFIER = "%li";
    template<> const wchar_t* StrTraits<wchar_t>::UINT64_T_IDENTIFIER = L"%I64u";
    template<> const wchar_t* StrTraits<wchar_t>::INT64_T_IDENTIFIER = L"%I64i";
    template<> const wchar_t* StrTraits<wchar_t>::LONG_IDENTIFIER = L"%li";
    Those might be the wrong format conversions for the intN_t types. Check out cinttypes.

Page 5 of 5 FirstFirst 12345
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