3 ms·

Honestly, it was mostly the concision that struck me at the time. Looking at the zipmap source now, I think it's more or less where my code tends to end up in t

by mikeyk 15y ago

Honestly, it was mostly the concision that struck me at the time. Looking at the zipmap source now, I think it's more or less where my code tends to end up in terms of comments--some motivation / higher-level comments at the top, and most functions have a comment before, hopefully with some explanation of any edge/NULL cases as well. Any open-source projects you'd point to as good examples of what looks readable to you? Always trying to improve my own coding habits as well.

cperciva 15y ago
I like to think that most of my recent code is pretty readable. The largest chunk of open source is my kivaloo data store (http://www.tarsnap.com/kivaloo.html http://www.tarsnap.com/kivaloo.html, browsable svn repository at http://code.google.com/p/kivaloo/source/browse/ http://code.google.com/p/kivaloo/source/browse/).
- qeorge 15y ago
  Very readable for something so dense. Nice work! One suggestion from the peanut gallery: In http://code.google.com/p/kivaloo/source/browse/trunk/lib/datastruct/kvhash.c http://code.google.com/p/kivaloo/source/browse/trunk/lib/dat..., at line 178 you have: if (rehash(H)) goto err0; That's because rehash returns 0 upon success, and -1 on failure. To me, that's very confusing, because it reads to be erroring upon success. When I read rehash's body, I learn that 0 means success and -1 means failure. I'd prefer one of two methods. First, you could return 1 on success, and keep returning something falsy on failure. Then the code would read: if (!rehash(H)) goto err0; Which I think is more readable. Perhaps even better would be to use a constant, i.e., if (rehash(H) != REHASH_SUCCESS) goto err0; although that can easily lead to a mess of redundant constants, or a headache managing them. Just my personal preference. Really nice code, thank you for sharing it.
  - cperciva 15y ago
    Very readable for something so dense. Nice work! Thanks! you could return 1 on success, and keep returning something falsy on failure I come from an OS background, so to me 0 is success and non-zero is failure. It doesn't really matter which convention a project uses as long as it's consistent, so I documented this in my /STYLE file: "In general, functions should return (int)(-1) or NULL to indicate error."
    - qeorge 15y ago
      Gotcha. I'm not an OS guy, and didn't know that convention. Thanks! Dumb question though: If rehash can only return -1 or 0, won't if(rehash(H)) always fail?
      - cperciva 15y ago
        Dumb question though... There's no such thing as a dumb question, only people too dumb to take every available opportunity to learn. ;-) In C, "if (foo)" means "if (foo != 0)" if foo has integer type, so that line means "if (rehash(H) != 0)" or (since rehash only returns 0 or -1) equivalently "if (rehash(H) == -1)".
        qeorge 15y ago
        Thanks so much Colin, I'm learning a lot today. :)
      - ralph 15y ago
        The reason for the convention is that there's often only one form of success, so 0 suffices, but there may be many causes of failure.
    - nkurz 15y ago
      Both you and Salvatore have very readable code. I'm uncomfortable with having both 'H' and 'h' in the same function, and with using 'l' as a variable, but both of these files are exemplary C. Still, although it may just be personal preference, I give the slight edge to Salvatore on clarity. I have no real problem with 0 for success, but I'm with 'qeorge' on this. How is someone looking at only this function call supposed to determine if rehash() returns an integer or a pointer? My first guess would have been that you were checking for a non-NULL pointer. The 'goto err' makes it relatively clear after the fact, but not at a glance. If you want to keep returning -1, I think a better convention would be 'if (rehash(H) < 0)', which makes it more clear that you expect an int and better implies an error condition. But I think even better would be to return KV_SUCCESS or KV_FAILURE (defined however you choose) and check explicitly. This would also let you remove a bunch of lines by getting rid of every comment next to every return statement! :)

by mikeyk 15y ago

cperciva 15y ago

I like to think that most of my recent code is pretty readable. The largest chunk of open source is my kivaloo data store (http://www.tarsnap.com/kivaloo.html http://www.tarsnap.com/kivaloo.html, browsable svn repository at http://code.google.com/p/kivaloo/source/browse/ http://code.google.com/p/kivaloo/source/browse/).

qeorge 15y ago
Very readable for something so dense. Nice work! One suggestion from the peanut gallery: In http://code.google.com/p/kivaloo/source/browse/trunk/lib/datastruct/kvhash.c http://code.google.com/p/kivaloo/source/browse/trunk/lib/dat..., at line 178 you have: if (rehash(H)) goto err0; That's because rehash returns 0 upon success, and -1 on failure. To me, that's very confusing, because it reads to be erroring upon success. When I read rehash's body, I learn that 0 means success and -1 means failure. I'd prefer one of two methods. First, you could return 1 on success, and keep returning something falsy on failure. Then the code would read: if (!rehash(H)) goto err0; Which I think is more readable. Perhaps even better would be to use a constant, i.e., if (rehash(H) != REHASH_SUCCESS) goto err0; although that can easily lead to a mess of redundant constants, or a headache managing them. Just my personal preference. Really nice code, thank you for sharing it.
- cperciva 15y ago
  Very readable for something so dense. Nice work! Thanks! you could return 1 on success, and keep returning something falsy on failure I come from an OS background, so to me 0 is success and non-zero is failure. It doesn't really matter which convention a project uses as long as it's consistent, so I documented this in my /STYLE file: "In general, functions should return (int)(-1) or NULL to indicate error."
  - qeorge 15y ago
    Gotcha. I'm not an OS guy, and didn't know that convention. Thanks! Dumb question though: If rehash can only return -1 or 0, won't if(rehash(H)) always fail?
    - cperciva 15y ago
      Dumb question though... There's no such thing as a dumb question, only people too dumb to take every available opportunity to learn. ;-) In C, "if (foo)" means "if (foo != 0)" if foo has integer type, so that line means "if (rehash(H) != 0)" or (since rehash only returns 0 or -1) equivalently "if (rehash(H) == -1)".
      - qeorge 15y ago
        Thanks so much Colin, I'm learning a lot today. :)
    - ralph 15y ago
      The reason for the convention is that there's often only one form of success, so 0 suffices, but there may be many causes of failure.
  - nkurz 15y ago
    Both you and Salvatore have very readable code. I'm uncomfortable with having both 'H' and 'h' in the same function, and with using 'l' as a variable, but both of these files are exemplary C. Still, although it may just be personal preference, I give the slight edge to Salvatore on clarity. I have no real problem with 0 for success, but I'm with 'qeorge' on this. How is someone looking at only this function call supposed to determine if rehash() returns an integer or a pointer? My first guess would have been that you were checking for a non-NULL pointer. The 'goto err' makes it relatively clear after the fact, but not at a glance. If you want to keep returning -1, I think a better convention would be 'if (rehash(H) < 0)', which makes it more clear that you expect an int and better implies an error condition. But I think even better would be to return KV_SUCCESS or KV_FAILURE (defined however you choose) and check explicitly. This would also let you remove a bunch of lines by getting rid of every comment next to every return statement! :)