Critique of code quality
in [C++]
|
Prev: How to reuse an existing stream with a different streambuf ?
Next: A quick question about Initializer-List in C++0x
From: Kenneth 'Bessarion' Boyd on 9 Jun 2010 04:06 On Jun 9, 12:06 pm, Peter Olcott <NoS...(a)OCR4Screen.com> wrote: > The problem might be that the enum is an int whereas what it must > initialize is a uint8_t. This is not a problem in this situation. The proposed enumeration for ActionCode has range 0-13, which *does* fit in a uint8_t; the real question is whether the target compiler requires a cast for the assignment. -- [ See http://www.gotw.ca/resources/clcm.htm for info about ] [ comp.lang.c++.moderated. First time posters: Do this! ]
From: Juan Pedro Bolivar Puente on 9 Jun 2010 04:04 >> >> a) A precalculated table wouldn't be any bigger. > > The table itself takes much more stack space than a std::vector. > If it is precalculated it is global and therefore not in the stack. >> b) Why on earth would you want it on the stack rather than static? > Data hiding within the function that needs it. I create all of my > classes small enough to fit on the stack, this way I never ever have to > do dynamic memory allocation myself. By never doing dynamic memory > allocation myself all of the possible errors associated with this can > not possibly occur. > You can hide it as static private global. But yes, you need a .cpp for that. >> b) It just seems more logical - When I see a class I start wondering >> what state it is holding and why. If you don't agree with this that's >> fine but then consider why we would have static members in C++ at all? > Where is static member data stored? > In the global storage. The global storage is not the stack, nor the heap, it is statically determined at compile time. > >> >> Consider the code bloat that you'd have if the compiler DID inline >> these and you had several calls!! > In my case it is probably a singleton, thus no possible code bloat. > I don't understand what you mean here. You have no instance of the Singleton design pattern in your code, if you meant that. And it would not affect this any way. The code bloat comes from the compiler duplicating function object code on every function call. >> >> In practice what most compilers will do with this example is that they >> will not inline it but they will create a copy of the used method in >> every compilation unit that uses it. At best you make a lot of extra >> work for the linker. > > It also allows me to keep all of my code in headers. > And allows the compilation times to be huge and and the generated binary files to be huge too. > > I have been a "C" programmer since K&R was the de facto standard. I took > up C++ more than a decade ago. > How did you write the functions in C? As static functions implemented in the .h? Or did you use .cpp but just did no function call type checking at all? -- [ See http://www.gotw.ca/resources/clcm.htm for info about ] [ comp.lang.c++.moderated. First time posters: Do this! ]
From: Nick Hounsome on 9 Jun 2010 04:06 On 9 June, 18:06, Peter Olcott <NoS...(a)OCR4Screen.com> wrote: > On 6/9/2010 8:25 AM, Nick Hounsome wrote: > > > > > On 8 June, 22:03, Peter Olcott<NoS...(a)OCR4Screen.com> wrote: > >> On 6/8/2010 12:44 PM, Nick Hounsome wrote: > > >>> On 8 June, 10:54, Peter Olcott<NoS...(a)OCR4Screen.com> wrote: > >>>> I am aiming to produce the best balance of speed and space efficiency > >>>> against reliability and maintainability placing a higher weight on the > >>>> latter criteria. > >>>> http://www.ocr4screen.com/UTF8.h > > >>>> If there are any improvements that can be made within the criteria > >>>> provided, or other improvements that do not detract from the above > >>>> criteria input would be welcomed. > > >>> 1) uint8_t and uint32_t are already standard types so why are you > >>> typedefing your own? > > >> Because they are not defined on my platform. MS VS 2008. > > > I guessed. MS are pretty useless here - It's not like it's difficult > > to provide an these so you have to wonder why they don't. > > I would just create my own std header and include that. > > >>> 2) Precalculate the state table - It is logically a static const > >>> State[][]. > > >> I did it this way so that it would be small enough to fit on the stack, > >> and also so that it could be included as a header file. > > > a) A precalculated table wouldn't be any bigger. > > The table itself takes much more stack space than a std::vector. > > > b) Why on earth would you want it on the stack rather than static? > > Data hiding within the function that needs it. I create all of my > classes small enough to fit on the stack, this way I never ever have to > do dynamic memory allocation myself. By never doing dynamic memory > allocation myself all of the possible errors associated with this can > not possibly occur. That's not the point - If it's a static const 2d array then there is no dynamic memory used and no stack memory either. A win on time and (stack) space. > > c) Why do you want it in a header? > > Tow files is redundant and messy. All of my code is in headers. Without going into details either thousands of C++ programmers are doing it wrong and you have had a blinding insight that nobody else has thought of before or just maybe they are right. > >>> 3) Eliminate all the global stuff - It's not comp.lang.c here. > > >> I don't know what you mean, everything is a member of a class. > > > all the const uint8_t have global scope > > OK what is the best way to do this? Move them into the class (as enums) > >>> 4) Your class is logically stateless hence methods (as well as the > >>> state array) should be probably be static > > >> Maybe, what is the advantage? > > > a) you wouldn't keep creating your state table for every instance. > > b) It just seems more logical - When I see a class I start wondering > > what state it is holding and why. If you don't agree with this that's > > fine but then consider why we would have static members in C++ at all? > > Where is static member data stored? Same place global data is stored. And if it's a const 2d array it will just be memory mapped in from the exe - You can't do better than that. > >>> 5) It is highly unlikely that these methods would be inlined by the > >>> compiler (inline is a hint not an order) and if they were it could be > >>> a space wasting disaster. Make them non-inline - The call overhead > >>> will be dwarfed by the processing in almost all cases. > > >> The compiler requires inlining if the methods are included in the > >> header. I hate creating the typically required pair of files it seems so > >> redundant. > > > The compiler is never REQUIRED to inline anything in fact the keyword > > "inline" will almost certainly go the way of "register" and be > > deprecated some time in the future. > > MS VS always inlines when you "suggest" it to. Are you sure? Have you dissassembled this? That's another black mark for MS then. > > Consider the code bloat that you'd have if the compiler DID inline > > these and you had several calls!! > > In my case it is probably a singleton, thus no possible code bloat. Why would you bother posting if you don't want to write good reusable code??? Some poor guy may oneday decide to use your class and unless he looks at the implementation he will not realize why his app is so big. > > Separate header/source files are a huge aid to C++ allowing you to > > separate interface from implementation. This makes compilation faster > > and allows binary updates of dynamic libraries without recompilation. > > I just can't get over how messy this redundancy is, especially for tiny > little classes. What redundancy? The only thing duplicated is the method signatures. > > In practice what most compilers will do with this example is that they > > will not inline it but they will create a copy of the used method in > > every compilation unit that uses it. At best you make a lot of extra > > work for the linker. > > It also allows me to keep all of my code in headers. Putting aside right or wrong, At code review you would be told to go back and change it in every company I have ever worked for. > > Your comments here make me think that you're probably a C# or Java > > programmer. Files have a very different role in C++ and until you > > understand the term "compilation unit" you are not really getting it. > > I have been a "C" programmer since K&R was the de facto standard. I took > up C++ more than a decade ago. Do you put everything in headers in C? > >>> 6) Use vector::reserve so that push_back wont do multiple > >>> reallocations/copies > > >> Yes. I had initially allocated the worst case memory, then I thought > >> that this wasted memory. When I benchmarked it on 100 instances of the > >> entire Unicode set, it only took about 30% more time, but, 50% less > >> memory. It might be a good idea to at least reserve() the best case > >> memory requirement. > > >>> 7) Template the methods and use iterators rather than collections - > >>> it's more flexible (you can keep the current signatures as overloads) > >>> 8) Why are you using uint8_t as a State instead of an enum??????? > >>> NextState = 0 !!!???? Same goes for ActionCode. > > >> Yes this is the best suggestion. > > The problem might be that the enum is an int whereas what it must > initialize is a uint8_t. Why? -- [ See http://www.gotw.ca/resources/clcm.htm for info about ] [ comp.lang.c++.moderated. First time posters: Do this! ]
From: Alf P. Steinbach on 10 Jun 2010 05:29 * Juan Pedro Bolivar Puente, on 09.06.2010 21:04: > >>> >>> a) A precalculated table wouldn't be any bigger. >> >> The table itself takes much more stack space than a std::vector. >> > > If it is precalculated it is global and therefore not in the stack. > >>> b) Why on earth would you want it on the stack rather than static? >> Data hiding within the function that needs it. I create all of my >> classes small enough to fit on the stack, this way I never ever have to >> do dynamic memory allocation myself. By never doing dynamic memory >> allocation myself all of the possible errors associated with this can >> not possibly occur. >> > > You can hide it as static private global. But yes, you need a .cpp for that. Happily, no, you don't need a separately compiled file for the definition. As Leigh Johnston remarked else-thread you can have it as a static member of a templated base class. I used to call that "the templated constant idiom", but since it's so very much less than well-known (I and now Leigh seem to be the only persons who point out the possibility where it's relevant) it hardly deserves to be called an idiom, so I don't know what to call it... Cheers & hth., - Alf -- blog at <url: http://alfps.wordpress.com> [ See http://www.gotw.ca/resources/clcm.htm for info about ] [ comp.lang.c++.moderated. First time posters: Do this! ]
From: Leigh Johnston on 10 Jun 2010 05:28
"Juan Pedro Bolivar Puente" <magnicida(a)gmail.com> wrote in message news:4C0FC612.1090901(a)gmail.com... > > The code bloat comes from the compiler duplicating function object code > on every function call. > >>> >>> In practice what most compilers will do with this example is that they >>> will not inline it but they will create a copy of the used method in >>> every compilation unit that uses it. At best you make a lot of extra >>> work for the linker. >> >> It also allows me to keep all of my code in headers. >> > > And allows the compilation times to be huge and and the generated binary > files to be huge too. > If the code/tables are part of a class template then binary size may not be larger than putting it into a .cpp as the linker should drop duplicate definitions. /Leigh -- [ See http://www.gotw.ca/resources/clcm.htm for info about ] [ comp.lang.c++.moderated. First time posters: Do this! ] |