3 ms·

Do you mind elaborating? I obsess over clean code, so any criticism is appreciated.

by chjj 5y ago

arcticbull 5y ago
It looks super clean to me, the only exception being for the love of all that's good and holy always brace your one-line if statements and loops. You don't want your own "goto fail" do you? ;) [1] I think you did a great job. [1] https://nakedsecurity.sophos.com/2014/02/24/anatomy-of-a-goto-fail-apples-ssl-bug-explained-plus-an-unofficial-patch/ https://nakedsecurity.sophos.com/2014/02/24/anatomy-of-a-got...
- chjj 5y ago
  > It looks super clean to me, the only exception being for the love of all that's good and holy always brace your one-line if statements and loops. > You don't want your own "goto fail" do you? ;) [1] Ah, that was my style 7+ years ago. Then I started regularly contributing to an open source project which did not add curly braces on one-liners. I wanted to match the style of the project despite it feeling unnatural to me. Within a few weeks, my style was changed forever (for better or for worse). > I think you did a great job. Thank you.
- kazinator 5y ago
  Won't necessarily work: if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) { goto fail; } { goto fail; /* MISTAKE! THIS BLOCK SHOULD NOT BE HERE */ } Copy and paste bugs can play out in any number of ways. Newer GCC will catch the goto fail by noting that the indentation is wrong. $ gcc-11 -Wall gotofail.c gotofail.c: In function ‘main’: gotofail.c:5:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation] 5 | if (argc > 42) | ^~ gotofail.c:7:5: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’ 7 | goto fail; | ^~~~ $ cat gotofail.c #include <stdlib.h> int main(int argc, char **argv) { if (argc > 42) goto fail; goto fail; return 0; fail: return EXIT_FAILURE; } Of course, it could be that the indentation is not wrong. If somneone runs the code through some automatic formatting, the problem will not then be diagnosable that way. Sadly, if I fix the indentation then: $ gcc-11 -Wall -Wunreachable-code -O3 gotofail.c $ # silence even though the return 0; is not reachable. Still, I think I'm going to stick with "brace the else part if the then part is braced, and vice versa*.
MonkeyClub 5y ago
> I obsess over clean code Yep, it shows. I got curious over the GP's comment, and I spot checked json.c[1] and json.h[2]. They're cleanly written, your data structures reflect your use and I can get an idea of what does what through the code alone. Trying to figure out what could be considered obscure. For example would if (!btc_amount_import(&x, obj->u.string.ptr)) return 0; be considered obscure? Cause if you know the structure of a json_value, then you get what the phrase says. Well thought out and cleanly written code, I'd say. [1] https://github.com/chjj/mako/blob/master/src/json.c https://github.com/chjj/mako/blob/master/src/json.c [2] https://github.com/chjj/mako/blob/master/include/mako/json.h https://github.com/chjj/mako/blob/master/include/mako/json.h
- chjj 5y ago
  Not sure. I suppose I could add a style or coding convention guide somewhere in the repo to code that may seem confusing initially. Basic rules are: return values on the left side of params, and functions generally return a boolean for success or failure. The function you mention is parsing a fixed-point integer string (from a json string) and returning an int64_t. It will return 0 if it's not a syntactically valid integer, or if there is some kind of overflow, etc.
Jach 5y ago
I wasn't originally going to look at it but this claim got me intrigued. I'd say compared to a lot of C code I've seen, yeah, you aren't lying, the average cleanliness is so clean it's sick ;) Well done! Perhaps the GP means the general crypto nature of the project lends to opaqueness. For example, I randomly clicked into https://github.com/chjj/mako/blob/master/src/crypto/chacha20.c#L41 https://github.com/chjj/mako/blob/master/src/crypto/chacha20... and one could ask where the magic numbers on lines 41-44 come from. Maybe it's explained in the references, or just specified without explanation as part of the protocol. I looked at the reference implementation at https://cr.yp.to/streamciphers/timings/estreambench/submissions/salsa20/chacha8/ref/chacha.c https://cr.yp.to/streamciphers/timings/estreambench/submissi... which has the equivalent line at L66. After finding the implementation of U8TO32_LITTLE which does some bitwise-or'ing and shifting of the first 4 items ("expa" for either sigma or tau), in Lisp I quickly verified: (format nil "0x~x" (logior (char-code #\e) (ash (char-code #\x) 8) (ash (char-code #\p) 16) (ash (char-code #\a) 24))) ;-> "0x61707865" So perhaps an argument could be made that your implementation could be slightly cleaner by using those sigma/tau constants instead of the magic numbers for the first four entries in the initial state, which would show how they're related, but that doesn't really take away the magic-ness of them, just moves the question elsewhere to why those magic strings. And it wouldn't surprise me if the answer is just the strings were arbitrarily chosen; the sigma/tau naming in the reference suggests a common domain convention to me (I'm not a crypto guy though so I don't know). Still overall this is a minor point, it looks like a clean piece of code (especially given what I'm used to seeing from browsing other bits of crypto code here and there) and random sampling of other parts of the program are at least as nice and nicer, especially when they're not doing anything complicated, which I've seen plenty of C code make a mess of. While it might not always be clear why something is done, it's at least clear what's going on and where to jump for more context, what the interfaces are, and things are well-named such that I could probably go find references for some of the whys if they actually needed to be answered. e.g. in mempool.c's btc_mempool_verify, I have no clue what "Annoying process known as sigops counting" refers to, but even without that comment, the simple if condition's pieces are well-named so I could go search for more info on sigops, which I wouldn't even expect to be part of the code (at least here) anyway. (Edit: It also occurs to me that a low ratio of comments and explanation to code could also be what is meant by obfuscation. To me that's not a large part of the cleanliness concept, though it does factor into other qualities. I consider the sqlite codebase to be some of the most beautiful C code on the planet, wonderfully documented and tested, and as a whole its beauty more than makes up for some stylistic or organizational quirks I don't exactly like. Not to say that style is totally unimportant, but neither this nor sqlite are exemplars of ugly C with lots of insane typedefs, super macros, inconsistencies, syntax abuses everywhere, and mngld_nms like vowels cost $100 a pop. For a project I consider "not so good" C code, I reference Enlightenment Foundation Libraries...)

Do you mind elaborating? I obsess over clean code, so any criticism is appreciated.

by chjj 5y ago

Do you mind elaborating? I obsess over clean code, so any criticism is appreciated.

It looks super clean to me, the only exception being for the love of all that's good and holy always brace your one-line if statements and loops. You don't want your own "goto fail" do you? ;) [1] I think you did a great job. [1] https://nakedsecurity.sophos.com/2014/02/24/anatomy-of-a-goto-fail-apples-ssl-bug-explained-plus-an-unofficial-patch/ https://nakedsecurity.sophos.com/2014/02/24/anatomy-of-a-got...

chjj 5y ago
> It looks super clean to me, the only exception being for the love of all that's good and holy always brace your one-line if statements and loops. > You don't want your own "goto fail" do you? ;) [1] Ah, that was my style 7+ years ago. Then I started regularly contributing to an open source project which did not add curly braces on one-liners. I wanted to match the style of the project despite it feeling unnatural to me. Within a few weeks, my style was changed forever (for better or for worse). > I think you did a great job. Thank you.
kazinator 5y ago
Won't necessarily work: if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) { goto fail; } { goto fail; /* MISTAKE! THIS BLOCK SHOULD NOT BE HERE */ } Copy and paste bugs can play out in any number of ways. Newer GCC will catch the goto fail by noting that the indentation is wrong. $ gcc-11 -Wall gotofail.c gotofail.c: In function ‘main’: gotofail.c:5:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation] 5 | if (argc > 42) | ^~ gotofail.c:7:5: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’ 7 | goto fail; | ^~~~ $ cat gotofail.c #include <stdlib.h> int main(int argc, char **argv) { if (argc > 42) goto fail; goto fail; return 0; fail: return EXIT_FAILURE; } Of course, it could be that the indentation is not wrong. If somneone runs the code through some automatic formatting, the problem will not then be diagnosable that way. Sadly, if I fix the indentation then: $ gcc-11 -Wall -Wunreachable-code -O3 gotofail.c $ # silence even though the return 0; is not reachable. Still, I think I'm going to stick with "brace the else part if the then part is braced, and vice versa*.

MonkeyClub 5y ago

> I obsess over clean code Yep, it shows. I got curious over the GP's comment, and I spot checked json.c[1] and json.h[2]. They're cleanly written, your data structures reflect your use and I can get an idea of what does what through the code alone. Trying to figure out what could be considered obscure. For example would if (!btc_amount_import(&x, obj->u.string.ptr)) return 0; be considered obscure? Cause if you know the structure of a json_value, then you get what the phrase says. Well thought out and cleanly written code, I'd say. [1] https://github.com/chjj/mako/blob/master/src/json.c https://github.com/chjj/mako/blob/master/src/json.c [2] https://github.com/chjj/mako/blob/master/include/mako/json.h https://github.com/chjj/mako/blob/master/include/mako/json.h

chjj 5y ago
Not sure. I suppose I could add a style or coding convention guide somewhere in the repo to code that may seem confusing initially. Basic rules are: return values on the left side of params, and functions generally return a boolean for success or failure. The function you mention is parsing a fixed-point integer string (from a json string) and returning an int64_t. It will return 0 if it's not a syntactically valid integer, or if there is some kind of overflow, etc.

Jach 5y ago

I wasn't originally going to look at it but this claim got me intrigued. I'd say compared to a lot of C code I've seen, yeah, you aren't lying, the average cleanliness is so clean it's sick ;) Well done! Perhaps the GP means the general crypto nature of the project lends to opaqueness. For example, I randomly clicked into https://github.com/chjj/mako/blob/master/src/crypto/chacha20.c#L41 https://github.com/chjj/mako/blob/master/src/crypto/chacha20... and one could ask where the magic numbers on lines 41-44 come from. Maybe it's explained in the references, or just specified without explanation as part of the protocol. I looked at the reference implementation at https://cr.yp.to/streamciphers/timings/estreambench/submissions/salsa20/chacha8/ref/chacha.c https://cr.yp.to/streamciphers/timings/estreambench/submissi... which has the equivalent line at L66. After finding the implementation of U8TO32_LITTLE which does some bitwise-or'ing and shifting of the first 4 items ("expa" for either sigma or tau), in Lisp I quickly verified: (format nil "0x~x" (logior (char-code #\e) (ash (char-code #\x) 8) (ash (char-code #\p) 16) (ash (char-code #\a) 24))) ;-> "0x61707865" So perhaps an argument could be made that your implementation could be slightly cleaner by using those sigma/tau constants instead of the magic numbers for the first four entries in the initial state, which would show how they're related, but that doesn't really take away the magic-ness of them, just moves the question elsewhere to why those magic strings. And it wouldn't surprise me if the answer is just the strings were arbitrarily chosen; the sigma/tau naming in the reference suggests a common domain convention to me (I'm not a crypto guy though so I don't know). Still overall this is a minor point, it looks like a clean piece of code (especially given what I'm used to seeing from browsing other bits of crypto code here and there) and random sampling of other parts of the program are at least as nice and nicer, especially when they're not doing anything complicated, which I've seen plenty of C code make a mess of. While it might not always be clear why something is done, it's at least clear what's going on and where to jump for more context, what the interfaces are, and things are well-named such that I could probably go find references for some of the whys if they actually needed to be answered. e.g. in mempool.c's btc_mempool_verify, I have no clue what "Annoying process known as sigops counting" refers to, but even without that comment, the simple if condition's pieces are well-named so I could go search for more info on sigops, which I wouldn't even expect to be part of the code (at least here) anyway. (Edit: It also occurs to me that a low ratio of comments and explanation to code could also be what is meant by obfuscation. To me that's not a large part of the cleanliness concept, though it does factor into other qualities. I consider the sqlite codebase to be some of the most beautiful C code on the planet, wonderfully documented and tested, and as a whole its beauty more than makes up for some stylistic or organizational quirks I don't exactly like. Not to say that style is totally unimportant, but neither this nor sqlite are exemplars of ugly C with lots of insane typedefs, super macros, inconsistencies, syntax abuses everywhere, and mngld_nms like vowels cost $100 a pop. For a project I consider "not so good" C code, I reference Enlightenment Foundation Libraries...)