Ideas for how to improve code

Started by
58 comments, last by wood_brian 11 years, 10 months ago

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.

In time the project grows, the ignorance of its devs it shows, with many a convoluted function, it plunges into deep compunction, the price of failure is high, Washu's mirth is nigh.

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

Wielder of the Sacred Wands
[Work - ArenaNet] [Epoch Language] [Scribblings]

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();
}
"In order to understand recursion, you must first understand recursion."
My website dedicated to sorting algorithms

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.
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.
I will let the others help you with the functional quality of your code.


L. Spiro

I restore Nintendo 64 video-game OST’s into HD! https://www.youtube.com/channel/UCCtX_wedtZ5BoyQBXEhnVZw/playlists?view=1&sort=lad&flow=grid


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.

Which brings up the next few items on my list:

  • lil_string is horribly written. It exists for no purpose, and should be dropped in favor of std::string.

    • Your marshalling is O(3n) while std::string does it in O(n) (or less if you just memcpy blast).
    • The explicit constructor on lil_string takes any type and thus will produce compilation errors, or worse, WON'T produce them in some cases, introducing subtle bugs.
    • Construction of an empty lil_string involves a memory allocation.
    • Construction of a string with a C-string is O(2n).
    • Calculating the size of a string is O(n), std::string is O(1).
    • Assignment of a c-string is O(2n), std::string is O(n).
    • Assignment of a "lil_string" to another lil_string is flat out broken. in std::string it works as expected.
    • operator += runs in O(2n), std::string is O(n).
    • clear requires a memory allocation.
  • Passwords are sent plain text.
  • No two factor authentication.
  • Using strtok.
  • In time the project grows, the ignorance of its devs it shows, with many a convoluted function, it plunges into deep compunction, the price of failure is high, Washu's mirth is nigh.


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


    Last month you said "code generation is not hard."
    The code with the array of size 1 works fine. There's an example on my home page where the size is 2.


    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.
    [/quote]

    Thank you for the ideas that I've snipped for now. The C++ Middleware Writer(CMW) is an on line alternative to serialization libraries. This page lists some of the advantages the CMW has over the Boost serialization library. One advantage is the on line aspect. The amount of code that has to be downloaded and maintained (patched) by users is orders of magnitude less with the CMW than the Boost library. The traditional approach requires lots of people to maintain their installations. The on line approach allows for multiple versions of the CMW to be available (there's only one at the moment) and we patch those versions here.

    One advantage is the on line aspect.


    That's odd; this is the top of the list of why I would never consider this product.


    Which brings up the next few items on my list:

    • lil_string is horribly written. It exists for no purpose, and should be dropped in favor of std::string.

      • Your marshalling is O(3n) while std::string does it in O(n) (or less if you just memcpy blast).
      • The explicit constructor on lil_string takes any type and thus will produce compilation errors, or worse, WON'T produce them in some cases, introducing subtle bugs.
      • Construction of an empty lil_string involves a memory allocation.
      • Construction of a string with a C-string is O(2n).
      • Calculating the size of a string is O(n), std::string is O(1).
      • Assignment of a c-string is O(2n), std::string is O(n).
      • Assignment of a "lil_string" to another lil_string is flat out broken. in std::string it works as expected.
      • operator += runs in O(2n), std::string is O(n).
      • clear requires a memory allocation.
  • Passwords are sent plain text.
  • No two factor authentication.
  • Using strtok.


  • I've decided to take your advice and stop using lil_string. I wasn't using it much so it was easy to convert the code to use ::std::string.
    Yes, the stream isn't encrypted. I've been thinking about taking this approach as a first step toward improving things in that regard. I'm using compression already between the back and middle tiers.
    I don't know much about two-factor authentication... that looks like something to consider after addressing the other items that have been mentioned.

    [quote name='wood_brian' timestamp='1336157475' post='4937448']
    One advantage is the on line aspect.


    That's odd; this is the top of the list of why I would never consider this product.
    [/quote]

    We probably have two or three concepts mixed together here. There's the on line aspect, the remote/security aspect and the control/trust aspect. I'd guess that you are leery of the remote/security aspect and/or the control/trust aspect. I'm open to selling the right to use a private copy of the back tier to those interested in that. I think that addresses the remote/security and control/trust aspects. I'm primarily interested, though, in providing free users with heavenly service. That's the future.

    This topic is closed to new replies.

    Advertisement