# Ideas for how to improve code

This topic is 2047 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## [font=UsherwoodStd-Medium][font=UsherwoodStd-Medium]http://webEbenezer.n...ntegration.html . One area that needs work is settling on a coding convention and adhering to it -- the names of classes/functions/files aren't consistent. Thoughts on that are appreciated, but I hope there will be other ideas. Tia.[/font][/font]

##### Share on other sites
Consistency is nice, but as long as everything is relatively readable, it should be the least of one's concerns, IMHO.

I haven't looked very hard. Without spending a long time trying to figure out it's purpose, it's hard to be sure if it's fit for purpose. Also, a lot of it is headers without any implementation, so what I'm not sure what you're hoping to get out of this.

But...

I don't see any tests. bug_count(code without tests) >= bug_count(same code with tests). So that's one easy way of improving the code.

The "[color=#000000]malloc_never_null" function is also extremely weird, especially since it's used in a context where you know there's a maximum buffer size of 255 (which is peanuts for almost any system).

[color=#000000]I counted a grand total of 2 asserts

##### Share on other sites
 /* * Like new, we want to guarantee that we NEVER * return NULL. Loop until there is free memory. * */ static char* malloc_never_null (size_t const b) { char *p; do { p = static_cast<char*>(malloc(b)); } while ( p == NULL ); return p; }

This is just wrong, you should feel ashamed.. the reason new never returns a null pointer is because it throws an exception if it can't allocate the requested amount of memory.

Unless you disable exceptions in your specific compiler, then new can of course return null pointers (depending on implementation specifics).

##### Share on other sites

This is just wrong, you should feel ashamed.

There's no shame in doing something incorrectly.

##### Share on other sites

the reason new never returns a null pointer is because it throws an exception if it can't allocate the requested amount of memory.

Unless you disable exceptions in your specific compiler, then new can of course return null pointers (depending on implementation specifics).

To expand on this, [font=courier new,courier,monospace]new [/font]can be made to return [font=courier new,courier,monospace]NULL[/font]/[font=courier new,courier,monospace]nullptr [/font]upon failure if you use [font=courier new,courier,monospace]nothrow[/font]. In which case, [font=courier new,courier,monospace]new [/font]may return [font=courier new,courier,monospace]NULL[/font]. If there isn't any available memory when you request it, there probably won't be any the next time you request it 700 nanoseconds later, and there isn't much sense in getting stuck in a potentially infinite loop.

I'm curious as to why you're using [font=courier new,courier,monospace]malloc [/font]instead of [font=courier new,courier,monospace]new[/font].

While edd[sup]2[/sup] has a point about consistency not being critical to having a working product, I will say that I would be horribly frustrated if a project was inconsistent in its conventions.

This line (from MarshallingFunctions.hh):
(boolRep != 0) ? bl = true : bl = false;
is technically valid, but it goes against every normal use of the ternary operator. If you wanted it to be consistent with how the other 99.99% of the world programs, it would be:
bl = boolRep != 0;
or at most:
bl = (boolRep != 0) ? true : false;
If you really wanted to do it that way though, you could also do
if (boolRep != 0) b1 = true; else b1 = false;
which is slightly common (but depends on your coding conventions), but I'd say by far the most common are the first two I mentioned.

I will commend you on seeking to improve your code. It's not always easy to ask for a critical review. Edited by Cornstalks

##### Share on other sites

There's no shame in doing something incorrectly.

There's no good reason to use malloc in C++ anyway, especially if you're about to use it incorrectly.

If you really want "performance" (PLEASE PROFILE FIRST), then you can use the OS-specific HeapRealloc or POSIX realloc APIs and hide them behind an overloaded new operator.

##### Share on other sites

[quote name='Mihai Moldovan' timestamp='1335828424' post='4936216']
This is just wrong, you should feel ashamed.

There's no shame in doing something incorrectly.
[/quote]
+1. The only shame lies in not learning why it was incorrect.

##### Share on other sites

[quote name='Mihai Moldovan' timestamp='1335828424' post='4936216']
the reason new never returns a null pointer is because it throws an exception if it can't allocate the requested amount of memory.

Unless you disable exceptions in your specific compiler, then new can of course return null pointers (depending on implementation specifics).

To expand on this, [font=courier new, courier, monospace]new [/font]can be made to return [font=courier new, courier, monospace]NULL[/font]/[font=courier new, courier, monospace]nullptr [/font]upon failure if you use [font=courier new, courier, monospace]nothrow[/font]. In which case, [font=courier new, courier, monospace]new [/font]may return [font=courier new, courier, monospace]NULL[/font]. If there isn't any available memory when you request it, there probably won't be any the next time you request it 700 nanoseconds later, and there isn't much sense in getting stuck in a potentially infinite loop.

I'm curious as to why you're using [font=courier new, courier, monospace]malloc [/font]instead of [font=courier new, courier, monospace]new[/font].
[/quote]

I picked that code up from someone on line and haven't paid enough attention to it yet. It probably should use new.

While edd[sup]2[/sup] has a point about consistency not being critical to having a working product, I will say that I would be horribly frustrated if a project was inconsistent in its conventions.
[/quote]

I agree. It would be too arbitrary otherwise. What I may do is adopt one naming convention for my library and the generated code and another naming convention for the executables. I'm not really comfortable with lower case and underscores exclusively or all camel case. I'm going to work on the convention for the library first.

This line (from MarshallingFunctions.hh):
(boolRep != 0) ? bl = true : bl = false;
is technically valid, but it goes against every normal use of the ternary operator. If you wanted it to be consistent with how the other 99.99% of the world programs, it would be:
bl = boolRep != 0;
or at most:
bl = (boolRep != 0) ? true : false;
If you really wanted to do it that way though, you could also do
if (boolRep != 0) b1 = true; else b1 = false;
which is slightly common (but depends on your coding conventions), but I'd say by far the most common are the first two I mentioned.
[/quote]

I think I'll go with this one:
bl = (boolRep != 0) ? true : false;
This is a small thing, but these things add up. Edited by wood_brian

##### Share on other sites

Consistency is nice, but as long as everything is relatively readable, it should be the least of one's concerns, IMHO.

I haven't looked very hard. Without spending a long time trying to figure out it's purpose, it's hard to be sure if it's fit for purpose. Also, a lot of it is headers without any implementation, so what I'm not sure what you're hoping to get out of this.

There's an archive here that has all of the files.

But...

I don't see any tests. bug_count(code without tests) >= bug_count(same code with tests). So that's one easy way of improving the code.

The "[color=#000000]malloc_never_null" function is also extremely weird, especially since it's used in a context where you know there's a maximum buffer size of 255 (which is peanuts for almost any system).

[color=#000000]I counted a grand total of 2 asserts
[/quote]

You got it right. (There's more than two but they are in the segmented_array code by Leigh Johnston that I ship with the archive.) Perhaps you could show an example of where you would add an assert.

##### Share on other sites

Perhaps you could show an example of where you would add an assert.

As one example, you could be pedantic by changing (from MarshallingFunctions.hh)
inline void CstringCount (::cmw::Counter& cntr, char const* cstr) { marshalling_integer slen(::strlen(cstr)); slen.CalculateMarshallingSize(cntr); cntr.Add(slen.value); }
to:
inline void CstringCount (::cmw::Counter& cntr, char const* cstr) { assert(cstr != NULL); marshalling_integer slen(::strlen(cstr)); slen.CalculateMarshallingSize(cntr); cntr.Add(slen.value); }

Personally, I like to use [font=courier new,courier,monospace]assert[/font] whenever i have to deal with a pointer.

##### Share on other sites

bl = (boolRep != 0) ? true : false;
which is slightly common

Really?
I've actually seen code like that several places, in the worst case (imo), like this:
return isEnabled ? true : false;
Which of course is equivalent to
return isEnabled;
The only different is that the first one is harder to understand, and takes longer to type. Edited by boblehest

##### Share on other sites

bl = (boolRep != 0) ? true : false;[/quote]

Idiomatic approach, one accepted in C (standard for embedded) as well as many other languages.

b1 = !!boolrep;! obviously stands for negation.

Result of the expression is a true boolean value and it works even without explicit bool type. As a common idiom, it also behaves intuitively.

##### Share on other sites

Idiomatic approach, one accepted in C (standard for embedded) as well as many other languages.

b1 = !!boolrep;! obviously stands for negation.

Result of the expression is a true boolean value and it works even without explicit bool type. As a common idiom, it also behaves intuitively.

OK, I'll give that a shot.

Feel free to make C++ 11 recommendations.

Currently I have this:

 #ifdef ENDIAN_BIG , byteOrder(most_significant_first) #else , byteOrder(least_significant_first) #endif 

 #ifdef ENDIAN_BIG ::cmw::ReceiveBufferTCPCompressed<LeastSignificantFirst> cmwBuf; #else ::cmw::ReceiveBufferTCPCompressed<SameFormat> cmwBuf; #endif uint8_t const byteOrder; 

I want to rewrite that as the following in the .hh file.

 #ifdef ENDIAN_BIG ::cmw::ReceiveBufferTCPCompressed<LeastSignificantFirst> cmwBuf; uint8_t const byteOrder = most_significant_first; #else ::cmw::ReceiveBufferTCPCompressed<SameFormat> cmwBuf; uint8_t const byteOrder = least_significant_first; #endif 

But VS 11 doesn't have support for that yet. I hope Microsoft will add support for that soon. Edited by wood_brian

##### Share on other sites

Perhaps you could show an example of where you would add an assert.

Everywhere you assume something. Really that's your job!

Some examples, though:

• [color=#000000]direct.cc, same again
• [color=#000000]lil_string.h, [color=#000000]strdup_never_null, assert(s != 0);
• [color=#000000]same again in lil_string constructor
• [color=#000000]...

[color=#000000]There are probably higher-level invariants that you're more aware of than I am, given you know what it's meant to be doing.

[color=#000000]While I flick through the code again, I notice that you tend not to initialize variables that are immediately passed as references to functions that fill them in. I always think it's a good idea to initialize everything, as any place where you introduce the possibility for the behaviour of a program to differ between debug and optimized builds is asking for trouble; while debugging I'd take an impossible '0' or '-1' over an unexpected-but-entirely-plausible '273981' any day. The C++ standard gives the programmer quite enough rope in this regard already, IMO.

##### Share on other sites

Everywhere you assume something. Really that's your job!

Some examples, though:

• [color=#000000]direct.cc, same again

[color=#000000]

[color=#000000]Here's the code for that function
[color=#000000] void getaddrinfo_wrapper(char const * node , int port , addrinfo** res ) { ::std::stringstream out; out << port; addrinfo hints; ::std::memset(&hints, 0, sizeof(hints)); hints.ai_family = AF_UNSPEC; hints.ai_socktype = SOCK_STREAM; int err; if ((err = getaddrinfo(node , out.str().c_str() , &hints , res )) != 0) { throw failure("Getaddrinfo: ") << gai_strerror(err); } } 

Something is wrong with getaddrinfo if it doesn't set res and it doesn't fail.

• [color=#000000]While I flick through the code again, I notice that you tend not to initialize variables that are immediately passed as references to functions that fill them in. I always think it's a good idea to initialize everything, as any place where you introduce the possibility for the behaviour of a program to differ between debug and optimized builds is asking for trouble; while debugging I'd take an impossible '0' or '-1' over an unexpected-but-entirely-plausible '273981' any day. The C++ standard gives the programmer quite enough rope in this regard already, IMO.

[/quote]
OK.

##### Share on other sites
malloc_never_null is flat out broken. if you ever encounter a situation where malloc does return null, it returns it for one reason: it could not allocate sufficient space.

new returns null if you pass it std::nothrow. If you do not then when it FAILS TO ALLOCATE it throws an exception. It does not infinitely loop.

Your function naming is atrocious, and leaves much to be desired. You mix case with underscores and some of the names are flat out confusing. There seems to be little divide between the public interface that you expect users to use and the private interface that you expect your internals to operate against. You are using C++, however you are not using C++ allocations (new) and using C style ones (malloc). You provide your own string implementation, which is worse than std::string in many ways and provides no inherit benefit to using it over that of just using std::string. Your use of pre-processor definitions is also terrible, with potential name collisions existing in trivial cases. Edited by Washu

##### Share on other sites

malloc_never_null is flat out broken. if you ever encounter a situation where malloc does return null, it returns it for one reason: it could not allocate sufficient space.

new returns null if you pass it std::nothrow. If you do not then when it FAILS TO ALLOCATE it throws an exception. It does not infinitely loop.

Your function naming is atrocious, and leaves much to be desired. You mix case with underscores and some of the names are flat out confusing.

What function names do you think are confusing?

There seems to be little divide between the public interface that you expect users to use and the private interface that you expect your internals to operate against. You are using C++, however you are not using C++ allocations (new) and using C style ones (malloc).
[/quote]
That's not true. The only file that uses malloc is lil_string.hh. I picked up that code on line and haven't changed it much except to make it throw an exception if the size of the string grows over 255.

You provide your own string implementation, which is worse than std::string in many ways and provides no inherit benefit to using it over that of just using std::string. Your use of pre-processor definitions is also terrible, with potential name collisions existing in trivial cases.
[/quote]

The purpose of the string class is limited, but if you know you don't need a string of length more than 255 then by using lil_string we can marshall the length of the string in one byte. What pre-processor definitions are you referring to?

##### Share on other sites

[quote name='Washu' timestamp='1336023282' post='4936971']
malloc_never_null is flat out broken. if you ever encounter a situation where malloc does return null, it returns it for one reason: it could not allocate sufficient space.

new returns null if you pass it std::nothrow. If you do not then when it FAILS TO ALLOCATE it throws an exception. It does not infinitely loop.

Your function naming is atrocious, and leaves much to be desired. You mix case with underscores and some of the names are flat out confusing.

What function names do you think are confusing?
[/quote]
Have you actually LOOKED at your function naming scheme? Here's a prime example of what's wrong with your code: http://webebenezer.net/misc/MarshallingFunctions.hh

Look at that mess, it's terrible. You have functions that start with lower case letters, you have ones that start with upper case letters, you have ones that contain UNDERSCORES in their names. Pick a style and stick with it.

There seems to be little divide between the public interface that you expect users to use and the private interface that you expect your internals to operate against. You are using C++, however you are not using C++ allocations (new) and using C style ones (malloc).
[/quote]
That's not true. The only file that uses malloc is lil_string.hh. I picked up that code on line and haven't changed it much except to make it throw an exception if the size of the string grows over 255.
[/quote]
That still doesn't fix the problem then. A malloc of ONE BYTE can fail if you're out of memory. In which case we're right back at the infinite loop scenario.

You provide your own string implementation, which is worse than std::string in many ways and provides no inherit benefit to using it over that of just using std::string. Your use of pre-processor definitions is also terrible, with potential name collisions existing in trivial cases.
[/quote]

The purpose of the string class is limited, but if you know you don't need a string of length more than 255 then by using lil_string we can marshall the length of the string in one byte. What pre-processor definitions are you referring to?
[/quote]
This has, quite literally, NOTHING AT ALL to do with implementing your own string class which is worse than std::string. In fact, that's entirely an ENCODING issue when writing the data and can be avoided entirely. Here's a hint: 7 bit integer encoding. Also, your lil_string class is in the global namespace, and not in...say the cmw namespace.

As for your preprocessor definitions... the issue is with ALL OF THEM. Firstly we have WIN_DOWS... not like CMW_WINDOWS or something that makes sense, just a random WIN_DOWS. Then we have your inclusion guards... yay! lowercase. Also not included in all of your headers. Basically, your existing inclusion guards and your WIN_DOWS preprocessor definitions are trivial cases for having a collision against user code definitions or third party libraries the user may be using. This is why, for instance, all BOOST preprocessor definitions start with "BOOST_". Edited by Washu

##### Share on other sites

Have you actually LOOKED at your function naming scheme? Here's a prime example of what's wrong with your code: http://webebenezer.n...ingFunctions.hh

Look at that mess, it's terrible. You have functions that start with lower case letters, you have ones that start with upper case letters, you have ones that contain UNDERSCORES in their names. Pick a style and stick with it.

I'm not arguing about that. I've already said it needs work. I mentioned that in the original post. You didn't answer my question about the function names that you find confusing.

That still doesn't fix the problem then. A malloc of ONE BYTE can fail if you're out of memory. In which case we're right back at the infinite loop scenario.
[/quote]
Understood. People don't seem to believe that I agree there are problems with this file and I plan on fixing them.

This has, quite literally, NOTHING AT ALL to do with implementing your own string class which is worse than std::string. In fact, that's entirely an ENCODING issue when writing the data and can be avoided entirely. Here's a hint: 7 bit integer encoding. Also, your lil_string class is in the global namespace, and not in...say the cmw namespace.
[/quote]

I'm already using 7 bit integer encoding --
http://webEbenezer.n...ling_integer.hh
http://webEbenezer.n...ling_integer.cc
MarshallingFunctions.hh uses marshalling_integer when it marshals ::std::string and char-based strings. No one is forced to use lil_string.

As for your preprocessor definitions... the issue is with ALL OF THEM. Firstly we have WIN_DOWS... not like CMW_WINDOWS or something that makes sense, just a random WIN_DOWS. Then we have your inclusion guards... yay! lowercase. Also not included in all of your headers. Basically, your existing inclusion guards and your WIN_DOWS preprocessor definitions are trivial cases for having a collision against user code definitions or third party libraries the user may be using. This is why, for instance, all BOOST preprocessor definitions start with "BOOST_".
[/quote]

Good point about the prefix. I'll work on changing those macro names.

##### Share on other sites

You didn't answer my question about the function names that you find confusing.
[/quote]
Oh, I need to frobnicate that foo. Should I write:

• frobnicateFoo?
• FrobnicateFoo?
• frobnicate_foo?
• Frobnicate_Foo?
• ...

This is intellectual overhead that benefits no one. In most libraries, if you can see at least one other function call you know the format of all of them. Not so if the naming convention is inconsistent.

I'm already using 7 bit integer encoding
[/quote]
Great! Unfortunately, this nullifies the only benefit of lil_string you've given:

... if you know you don't need a string of length more than 255 then by using lil_string we can marshall the length of the string in one byte.
[/quote]

##### Share on other sites

I'm already using 7 bit integer encoding

Great! Unfortunately, this nullifies the only benefit of lil_string you've given:

... if you know you don't need a string of length more than 255 then by using lil_string we can marshall the length of the string in one byte.
[/quote]
[/quote]
Not only that, but his string implementation is hideously written and will be a performance black hole for any serious use-case. Yet another reason why people who don't know what they're doing shouldn't try to implement standard containers.

##### Share on other sites
I didn't spend much time in the code - certainly not enough to comment on the architecture or overall design - but here's some quick impressions:

• You use an awful lot of magic numbers in your code. This is a very sloppy habit and makes me wary of the code quality as a whole. You should never have any magic number that isn't well documented in some way, and ideally about 95% of the numbers in your code should be removed - either to configuration settings, constants, or more robust algorithms (such as anything that touches the arguments to main).

• Naming conventions - or utter lack thereof... well, a lot's already been said on that in this thread, so I won't belabor the point. But this is a serious obstacle (IMHO) to anyone examining your code in any serious depth. It takes so much mental overhead to figure out what code is even supposed to do, because the names are useless, that I can't summon the energy at this point to try and do a higher-level analysis of the code.

• You seem to sprinkle around a lot of #ifdefs and platform-specific stuff in arbitrary places. You should be designing your code so that these concerns are totally encapsulated. Similarly, all of the socket stuff should be wrapped so you don't duplicate a bunch of logic all over the place. Logging deserves a wrapper as well. I'm sure there are more examples. It seems to me like you could benefit a lot from a few select extractions of functionality into centralized locations. There's some half-done work in this direction but it really ought to be carried to completion.

• try/catch(...) anywhere but at an extremely high level always worries me. Any instance of throw; immediately worries me even more. The combination is egregious. You should really reconsider your approach to exceptions. Also, I haven't confirmed this deeply, but I suspect that most of your code is highly unsafe in the presence of thrown exceptions.

• The machine generated code is kind of weird. I'm used to machine-genned code looking a little odd, but stuff like allocating a stack array of size 1 is just... ugh. If you're emitting that kind of code, it makes me very leery of what kind of other stuff your code generation is up to. Code generation is very hard, and it pays to make sure you're exhaustively careful with it and don't take easy shortcuts too often.

I'm sure I could dig up more, but that ought to keep you busy for long enough.

Also, despite your mentioning of this project on a frequent basis around here, I'm still utterly confused as to what exactly it's supposed to do. A real example beyond the one shown on the demo page would be nice, something that illustrates why I'd ever want to hand off my code to this system.

##### Share on other sites
Whilst there are plenty of things I could say are a problem (e.g. realloc used badly), I'll instead Make a suggestion, or rather a contribution.
Consider the following as a replacement body for the LeastSignificantFirst::Read funtion that reads a uint64_t:{ value = 0; for (int i=0; i < 8; ++i) value = (value << 8) | buf.Give(); }

##### Share on other sites

Great! Unfortunately, this nullifies the only benefit of lil_string you've given:

The range for lil_string is up to 255 and there's no need to do the math to figure out how many bytes are needed to marshall the string. I just have lil_string as an option. It isn't used by my library. It is only used in an executable. I use it to store password which have to be less than 255 characters.

As a followup to some earlier comments... I found 3 other places where I was doing something lame with the ternary operator and have fixed them. I've also added a CMW prefix to some of the macro names.

##### Share on other sites
Here is a long list of things to consider while writing (or generating) code:
http://lspiroengine.com/?p=126

Skip Naming Conventions and read the rest. I tried to explain the reasons behind many coding practices, not jut list them.
They won’t solve all your issues here; your code has some problems, but the same code—but cleaner/tidier—is still better code.

L. Spiro