• Advertisement
Sign in to follow this  

Ideas for how to improve code

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

If you intended to correct an error in the post then please contact us.

Recommended Posts

[font=UsherwoodStd-Medium][font=UsherwoodStd-Medium]"we need a greater appreciation of incremental (engineering) improvements with a relevance to real-world systems. “That’s just engineering, and we’re computer scientists” is an attitude we can’t afford." Bjarne Stroustrup[/font][/font]



[font=UsherwoodStd-Medium][font=UsherwoodStd-Medium]I agree with that and am posting here to ask for some ideas on how to improve the code here --[/font][/font]

[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 this post


Link to post
Share on other sites
Advertisement
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 this post


Link to post
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 this post


Link to post
Share on other sites

This is just wrong, you should feel ashamed.

There's no shame in doing something incorrectly.

Share this post


Link to post
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.

[edit]

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 this post


Link to post
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 this post


Link to post
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 this post


Link to post
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 this post


Link to post
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 sad.png
[/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 this post


Link to post
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 this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement