ivartj / bencoding

Exercise in making a bencoding library.
1 stars 0 forks source link

code review #1

Open oliverthered opened 11 years ago

oliverthered commented 11 years ago

github crashed on me last time I wrote this so here goes, oliverthered from wrongplanet in no perticular order. 1: Type constants are base 2 is there an external requirement for this or could they be an enum 2: get rid of the gotos and user break properly etc... 3: define your constants as named constants #DEFINES these are manily strings used for formating in sprintf or comparison operations or other constants, this will not only self document your code but it will make them easy to change if need be and have them all defined in one place for clarity and easy reading. 4: instead of doing switch(type) for functionality instead have a struct of functionpointers and a lookup table such that (something like) functions[type]->function_name(parameters) can be used for the individual functionality for each type instead of having the code either inline or a switch case statement. most large c projects do this as a form of object orientation, it will a: make sure the code is encapsulated for each operation for each type b: make your code extensible c: make you code self documenting 5: consider using memcpy instead of sprintf for constants as that's what your really doing and it's much more efficient 6: I haven't looked that well into the code but you may want some kind of stream system for buffer memory management, IDK.

apart from that not bad. ;-)

ivartj commented 11 years ago

This gives me food for though. Thanks!

There is no reason that the constants are powers of two other than the possibility of a reason.

oliverthered commented 11 years ago

this gives string steam support for c http://www.gnu.org/software/libc/manual/html_node/String-Streams.html

oliverthered commented 11 years ago

actually thats red only, this page gives more info on custom streams etc...... http://www.gnu.org/software/libc/manual/html_node/Other-Kinds-of-Streams.html#Other-Kinds-of-Streams

ivartj commented 11 years ago

Hmm... non-standard GNU extensions?

ivartj commented 11 years ago

I have added a (hopefully temporary) dependency to my personal I/O library so that the json writer isn't writing into a fixed-size buffer. This also means that it won't compile on Windows as the I/O library depends on vasprintf, which is not present on Windows.

ivartj commented 11 years ago

I have now managed to make the I/O library and bencoding library compile on Windows. I have also made htonl and ntohl functions for the crypto library so that it compiles on Windows without linking to Winsock, so the btutils package now works on Windows.