6 ms·

I'm not talking about correctness though. I'm talking about readability. There are so many (classical!) principles of readability (and debuggability) that are a

I'm not talking about correctness though. I'm talking about readability. There are so many (classical!) principles of readability (and debuggability) that are all being violated in that one function: - Not mixing pre and post increments - Not altering function arguments - Minimizing (not maximizing!!) side effects in conditionals - Putting braces around a loop body - Having fewer entry and exit points (lots of returns especially make debugging hell) - Just minimizing needless mutation in general etc. To give an example, I have such a hard time figuring out what even 'len' is in your code after the loop, and what it implies. I have to think about whether it's signed or unsigned, whether it might wrap around or not (I actually thought you had a bug until I saw the earlier return), whether it ends as zero or something else, and what its relationship to the pointers is. (e.g., whether they all change by exactly the same amount.) Whereas just using a simple index without modifying the arguments just erases all those problems altogether. It's one thing to feel they're both equally likely to be buggy (I actually don't particularly agree on that either, but I see what you're saying and think your viewpoint is fine there), but do you find them equally readable/understandable/debuggable too? Surely you can agree it's easier to read code that modifies just 1 local variable and has 1 return with just 1 extraneous increment than one that modifies 3 arguments simultaneously, misuses the natural point of a loop, and then adds several returns to handle cases with only minor variations? Btw, I'm not even particularly concerned about your ability to write code like this. I'm worried about propagating dangerous practices to other people. IMO, for people (or teams) who can write exceptionally correct code even in an unconventional manner, I just keep my mouth shut and let them do their thing as best as they can... internally. But seeing practices that are known to be error-prone for decades still being exhibited and propagated to us mere mortals who are likely to make predictable (human) errors is what bugs me. Especially when the very goal is to teach people to write safe and secure code!

userbinator 5y ago
There are so many (classical!) principles of readability (and debuggability) that are all being violated in that one function In other words, you're just repeating cargo-cult "I don't like how this looks" dogmatism and rejecting it outright, instead of making any attempt at actual understanding. I strongly recommend that everyone should try an APL-family language, even if only a little bit, to realise that there's a whole world of programming beyond the lowest-common-denominator tripe that's common these days. The mind needs to be exercised.
- deleted 5y ago
  [deleted]
saagarjha 5y ago
I just want you to know that I find your comments to be interesting, they're really forcing me to find words for why I wrote the function this way! Oh, and ignore 'userbinator; I'm pretty sure I understand their point but they know better than to be expressing their views like that. Anyways, to address your points: I suspect we are reading this function at different "levels". You see it as a mess of mutation and increment operators, but it is written that way on purpose! The reason for this are the "idioms" I mentioned above. You might be aware of some in C already, like struct foo f = { 0 }; To someone who is unfamiliar with the syntax this is can be confusing: "it's assigning 0 to the struct…what?!" Logically, this zero-initializes the first member of the struct, then implicitly zero-initializes all the other members. But an experienced reader isn't going to read it like that, they're going to read it like "clear out the struct"–they could probably tell you how the C standard makes this work and why this is better than memset, but in their brain when they read it they just know the idiom " = { 0 }" and what it does without going through all that. Similarly, in functional languages you might see code like let product = [1, 2, 3, 4].fold(*) which computes the product of all the numbers. Again, someone can explain how the whole thing works, and * is a binary operator that is being coerced into a closure argument, or whatever, but at some point you just see ".fold(*)" and think "product". At some point you may even prefer seeing ".fold(*)" over var product = 0 for i in [1, 2, 3, 4] { product *= i } because it's concise and doesn't leave room for error (did you spot the bug? I've done this more than once. It's easy to find and fix, but still, hopefully it illustrates how these kinds of things happen.) Now, with that in mind, we have some idioms in my code. These are what they are: while ((*dst++ = *src++)); I "know" this idiom as a strcpy. I also "know" that the loop invariant on this is that dst and src always point to the next character to copy: this is true at the start of the loop, while it's running, and at the end except at that point you're done copying so you don't want to run this loop anymore. I don't futz around with the increment operators, I just have internalized this to be true and read it as such. Similarly, while (--len); is just an idiom to run a loop len - 1 times (I made a typo in the first reply, heh). At the end len is zero because it's also the condition. With that done, I think the algorithm I implemented becomes clear: 1. If the buffer has no space, return early. (You can see this is a special case bailout–you'll see why it exists in just a second.) 2. Do a strcpy. But, before we copy any character, we need to also check to make sure you haven't run more than len - 1 times (Why? Again, see below). This check actually has to happen "first" so put it in front of the &&, to make sure we bail before writing a character. 3. Ok, so the loop is over. Since we have two conditions, this means there are two possible cases: either we hit the limit, or we finished copying the string. Actually there are three, since both could happen at once. If len is zero we ran out the buffer, so we need to null-terminate. Because we got rid of that pesky empty buffer case at the beginning, and we've copied len - 1 bytes, we have a guaranteed invariant here: we need to write one last NUL byte, and we have the space for it. Because we used the strcpy idiom, we know that dst has been pointing at the next spot to write to all this time, so we use it now. And src is pointing at the next character we want to write, so it lets us conveniently fill that case where the string fits perfectly, because we can figure this out from the input. The other side of the if is simpler, we simply finished copying without problems, so we can return dst as-is. Ok, that was a lot of words, but when I am reading it these stuff just comes automatically. This is why I brought up correctness: readability and correctness go hand-in-hand here. The readability principles you bring up are generally good ideas, but I chose to eschew them here precisely because the idioms give much powerful invariants that I think they are just more readable. Your function breaks out of the middle of a loop to ensure it doesn't overrun the string, mine just naturally rolls it into the loop condition. Your program increments dst past the end, goes "whoops", then goes back and writes in the NUL byte; mine only moves forwards and ensures that dst points to the place I want to write next from the moment the function starts all the way to the points of return. There is one edge case where I don't write a NUL (which 'saurik also noticed because he also tried to implement this function), but I bail out of that one immediately so the rest of the function can assume that it can write to the end of the buffer. So, in summary: I know how the increment idioms work. I don't see loops or mutation or side effects, I see the code as "this is the high-level thing that this sequence of characters will do", and then I structure my function around it so that the guarantees I know it can provide apply to my program.

userbinator 5y ago

There are so many (classical!) principles of readability (and debuggability) that are all being violated in that one function In other words, you're just repeating cargo-cult "I don't like how this looks" dogmatism and rejecting it outright, instead of making any attempt at actual understanding. I strongly recommend that everyone should try an APL-family language, even if only a little bit, to realise that there's a whole world of programming beyond the lowest-common-denominator tripe that's common these days. The mind needs to be exercised.

deleted 5y ago
[deleted]

saagarjha 5y ago

I just want you to know that I find your comments to be interesting, they're really forcing me to find words for why I wrote the function this way! Oh, and ignore 'userbinator; I'm pretty sure I understand their point but they know better than to be expressing their views like that. Anyways, to address your points: I suspect we are reading this function at different "levels". You see it as a mess of mutation and increment operators, but it is written that way on purpose! The reason for this are the "idioms" I mentioned above. You might be aware of some in C already, like struct foo f = { 0 }; To someone who is unfamiliar with the syntax this is can be confusing: "it's assigning 0 to the struct…what?!" Logically, this zero-initializes the first member of the struct, then implicitly zero-initializes all the other members. But an experienced reader isn't going to read it like that, they're going to read it like "clear out the struct"–they could probably tell you how the C standard makes this work and why this is better than memset, but in their brain when they read it they just know the idiom " = { 0 }" and what it does without going through all that. Similarly, in functional languages you might see code like let product = [1, 2, 3, 4].fold(*) which computes the product of all the numbers. Again, someone can explain how the whole thing works, and * is a binary operator that is being coerced into a closure argument, or whatever, but at some point you just see ".fold(*)" and think "product". At some point you may even prefer seeing ".fold(*)" over var product = 0 for i in [1, 2, 3, 4] { product *= i } because it's concise and doesn't leave room for error (did you spot the bug? I've done this more than once. It's easy to find and fix, but still, hopefully it illustrates how these kinds of things happen.) Now, with that in mind, we have some idioms in my code. These are what they are: while ((*dst++ = *src++)); I "know" this idiom as a strcpy. I also "know" that the loop invariant on this is that dst and src always point to the next character to copy: this is true at the start of the loop, while it's running, and at the end except at that point you're done copying so you don't want to run this loop anymore. I don't futz around with the increment operators, I just have internalized this to be true and read it as such. Similarly, while (--len); is just an idiom to run a loop len - 1 times (I made a typo in the first reply, heh). At the end len is zero because it's also the condition. With that done, I think the algorithm I implemented becomes clear: 1. If the buffer has no space, return early. (You can see this is a special case bailout–you'll see why it exists in just a second.) 2. Do a strcpy. But, before we copy any character, we need to also check to make sure you haven't run more than len - 1 times (Why? Again, see below). This check actually has to happen "first" so put it in front of the &&, to make sure we bail before writing a character. 3. Ok, so the loop is over. Since we have two conditions, this means there are two possible cases: either we hit the limit, or we finished copying the string. Actually there are three, since both could happen at once. If len is zero we ran out the buffer, so we need to null-terminate. Because we got rid of that pesky empty buffer case at the beginning, and we've copied len - 1 bytes, we have a guaranteed invariant here: we need to write one last NUL byte, and we have the space for it. Because we used the strcpy idiom, we know that dst has been pointing at the next spot to write to all this time, so we use it now. And src is pointing at the next character we want to write, so it lets us conveniently fill that case where the string fits perfectly, because we can figure this out from the input. The other side of the if is simpler, we simply finished copying without problems, so we can return dst as-is. Ok, that was a lot of words, but when I am reading it these stuff just comes automatically. This is why I brought up correctness: readability and correctness go hand-in-hand here. The readability principles you bring up are generally good ideas, but I chose to eschew them here precisely because the idioms give much powerful invariants that I think they are just more readable. Your function breaks out of the middle of a loop to ensure it doesn't overrun the string, mine just naturally rolls it into the loop condition. Your program increments dst past the end, goes "whoops", then goes back and writes in the NUL byte; mine only moves forwards and ensures that dst points to the place I want to write next from the moment the function starts all the way to the points of return. There is one edge case where I don't write a NUL (which 'saurik also noticed because he also tried to implement this function), but I bail out of that one immediately so the rest of the function can assume that it can write to the end of the buffer. So, in summary: I know how the increment idioms work. I don't see loops or mutation or side effects, I see the code as "this is the high-level thing that this sequence of characters will do", and then I structure my function around it so that the guarantees I know it can provide apply to my program.