23

@vnp's code is solid and helpful, but his stringify_state_helper is a single-purpose function, and still leaves a degree of repetition and memory management in stringify_state. I'd rather have general-purpose to_string that takes printf-style arguments, allocates sufficient space for the converted result, and prints into that space, and returns the result: #...


11

Consider factoring it out into a function: static size_t stringify_state_helper(State * state, char * buf) { return snprintf(buf, ....); } char * stringify_state(State * state) { size_t len = stringify_state_helper(state, NULL); char * buf = malloc(len + 1); if (buf) { stringify_state_helper(state, buf); } return buf; }


9

The GNU libc already provides asprintf, which avoids exactly the repetition you are concerned about. I haven't looked at the solutions of other operating systems (Windows, BSD, AIX, Solaris, embedded), though I hope they provide something equivalent.


7

Write the ifs such that the error paths are inside them, and the error-free path isn't indented: buff = malloc(len + 1); if (buff) { snprintf(buff, len + 1, format_string, state->last_update_time, p->health, p->max_health, p->satiation, p->max_satiation, ...


7

A few general tips: Have a function name that indicates what the function does. If it's hard to come up with a name that describes the function's purpose (e.g. because it does totally different things depending on what argument you pass it), it's a clue that the function needs to be broken up into multiple functions with clearer purposes. It looks like ...


6

Expanding on my comment; below you'll find a very simple program that compares the method I suggested with your original example. The results on my machine show that the MemoryMarshal class is about 85x faster. You might want to experiment a bit and try running a similar test with a larger struct; maybe your method is faster for the specific problem that you'...


5

You have both IEnumerableStreamSerializer and EnumerableStreamSerializer, and in fact neither of them are used in the code you posted. I'd have just stuck with one or the other (probably the interface, and introduce the base class if you need to share common functionality; alternatively if you need to worry about backwards compatibility with other people ...


5

I challenge the need for a list of booleans. There are alternatives available in the .NET Framework that deal with a sequence of booleans. If the flags are static and fixed: [Flags] enum IGreetYou : uint { None = 0, Hi = 1 << 0, Hello = 1 << 1, Yow = 1 << 2 } with a simple conversion to string as: var value = ...


5

Ok, for starters, it mentions on the first challenge: Always operate on raw bytes, never on encoded strings. Only use hex and base64 for pretty-printing. Since String is actually a list of encoded Chars, it would make more sense to migrate to an implementation that uses ByteString instead. This data structure is suited for handling raw data, like we will ...


5

This let filePath = Bundle.main.path(forResource: "test", ofType: "dat") guard filePath != nil else { return } let fileURL = URL(fileURLWithPath: filePath!) is an anti-pattern in Swift, see also When should I compare an optional value to nil? on Stack Overflow. Instead of testing a value against nil and then force-unwrapping it, you should use optional ...


3

A major drawback of this design is that you cannot reuse a single instance of structure.structure to read multiple records. Instead of s.fill(file_ptr), consider making a method s.read(f) that returns an OrderedDict of the deserialized data. (By the way, mentioning "pointer" in a parameter name isn't very Pythonic.) Note that that would also replace your ....


3

Use a common protocol file Any common constants, such as 9600 SYN, ACK, etc. strings should be #defined in one place, in this case a .h file accessible to both Arduino projects. Indentation One-space indentation is essentially never used. Typically four is used instead. Globals In the master code, every single one of those globals should be moved to ...


3

You implemented convertByteArrayToHexString really inefficiently because you allocate a new string for each byte that is converted. Also, there's no need for a StringBuilder since a character array suffices. The idiomatic code for converting a byte array to a hex string is: public static String toHex(byte[] bytes) { char[] chars = new char[2 * bytes....


3

To calculate number of bytes, better use: numbytes = math.ceil(value.bit_length() / 8) And then you can just: def be_to_le(value: int) -> int: numbytes = math.ceil(value.bit_length() / 8) return int.from_bytes(value.to_bytes(numbytes, byteorder="little"), byteorder="big")


3

I will focus on object-oriented programming in this review. Encapsulation From Wikipedia Encapsulation, in object-oriented programming, is the bundling of data with the methods that operate on that data, or the restricting of direct access to some of an objects components. Encapsulation is used to hide the values or state of a structured data object ...


3

Unnecessary Headers The #includes for iterator and bitset are not needed, the code compiles fine without them, iterator may be included indirectly through vector. Adding unnecessary headers can increase compile/build times and may cause other problems in larger more complex programs. Portability This program is currently not portable for 2 reasons, the use ...


3

This is a review of your 2nd review request, described as a rewrite for python3. Summary: code looks great! Much better than the older version. Many changes I suggested for the old already appear in the new. Thank you for turning mangling double __ into private single _ prefix. 0,aaa,b,c,dd (Bit Mapping) The explicit "bit mapping" remark is very ...


3

DFS (Directory and File Serialization) file format Not sure what that is. Nor is google. Recommend you offer a more specific citation, hopefully one that includes an URL. import os as _os import sys as _sys Hmmm, that's odd. Do you really need to rename them? FORMAT_DOCUMENTATION = '''\ The backwhack is very odd -- don't do it. That's what triple quotes ...


3

Separate pin definitions Clearer and easier to maintain. //#define SLAVEPINS 8, 9 //SoftwareSerial serial(SLAVEPINS); // RX, TX #define SLAVEPINS_RX 8 #define SLAVEPINS_TX 9 SoftwareSerial serial(SLAVEPINS_RX, SLAVEPINS_TX); Code guards #ifndef _SLAVE_H #define _SLAVE_H 1 #define SLAVEPINS 8, 9 #define SLAVEEVENT 200 .... #endif Also see #pragma ...


2

You should always use curly braces even if your loop/if statement contains 1 line. You're catching all Exceptions and assuming it's due to the file not being found. You should be catching FileNotfoundException instead. Alternatively you could check if the File is found or not. What if the property format is invalid? E.G 2 '=' signs or 0. Currently you ...


2

Here are some suggestions. __DYNAMICS_H__ is a reserved identifier because it contains consecutive underscores. Use a non-reserved identifier like DYNAMICS_H_. Also, this name is too common and may introduce name clashes. One solution is to append a sequence of random characters. I see no reason to make Clonables a friend of Clonable. The latter doesn't ...


2

In your current implementation you are caching the users in users field in UserManager, but this has a chance of data loss if there are multiple instances of the application is run and new user are created there. Password should always be hashed. Though I understand it is an internal app, this is a good practice to ensure anyone's password is not ...


2

I get that security isn't critical in this case, but it's probably still good to store the passwords in a hash. Some people may be using the same password for other websites and you don't want to compromise their security on those sites. There should be libraries to handle hashing passwords and it would be much better (and simpler) to use them than to try ...


1

BinaryFormatter is not adding any extra information. It does not store "here comes a double", this must be stored/handled by the reading algorithm. But to reproduce a certain byte order, you have to overwrite almost every serializing routine. At the end you gain nothing, comparing to your self-implementation. So it's not the extra-information, it's the ...


1

Race Condition You are starting two tasks concurrently, each blocking on an event to occur. var res = await Task.WhenAny(ReadAsync(stream, buffer, ct), GetPayload(ct)); By calling WhenAny, once one of tasks is completed, you continue without awaiting completion of the other task. Let's say that ReadAsync completes first, this means the following code rus ...


1

Side issue "%f" does not well express the state of a floating point (FP) object When a FP is large, printing 100s of digits is not informative. More compact options exist. Worse, when a FP is much smaller than 1, "%f" only retains a few or zero digits - losing perhaps all precession. As FP are encoded in exponential notation, use an exponential format ...


1

There's nothing wrong with the other answers, but if you'd prefer a solution more portable than asprinf() and slightly more straightforward than Jerry Coffin's vsnprintf() helper function solution, consider this variable length macro solution: #define FOO_ASPRINTF(_str, ...) /* replace FOO with your codebase's prefix */ \ do { \ int byte_c = snprintf(...


1

Returning early makes code not follow the arrow anti-pattern. And makes the flow of the code simpler. There's otherwise not much to comment on. class LoginSerializer(serializers.Serializer): phone = serializers.CharField() password = serializers.CharField() def validate(self, data): phone = data.get("phone", "") password = data....


1

Your code could be rewritten: var settings = new JsonSerializerSettings { TypeNameHandling = TypeNameHandling.All, SerializationBinder = knownTypesBinder // <- see security risk below }; var client = JsonConvert.DeserializeObject<Models.Entities.Client>(jsonResponse); return client; // <- strongly-typed Make sure both server and ...


1

Nesting Branches You were worried your code might be a bit ugly. I consider deeply nested branches ugly aka a penalty in readability when they can easily be avoided. For example, your function indents the entire while-loop body to perform this little branch at the end: if (byte xor IFM_FRAME_HEADER == BYTE_ZERO) { // branch body .. } else { index++ ...


Only top voted, non community-wiki answers of a minimum length are eligible