5 ms·

the gotos actually make sense in this case. unless you'd prefer some insane tree of if/else?

oneeyedpigeon 13y ago
Couldn't you just set a 'failed' boolean and wrap the fail: statements in a conditional check on it? Not that I'm saying all uses of goto, particularly this one, are necessarily absolute evil.
voidlogic 13y ago
I agree, I was thinking about what this situation would look like in other langs and when I turned to Go, I realized: While Go has goto for tricky situations like this, because it has defer you don't have to use it often, assuming the free calls were needed (and the vars were not going to be GC'd): defer SSLFreeBuffer(&hashCtx) defer SSLFreeBuffer(&signedHashes) if err = SSLHashSHA1.update(&hashCtx, &serverRandom); err != nil { return err; } else if err = SSLHashSHA1.update(&hashCtx, &signedParams); err != nil { return err; } else if err = SSLHashSHA1.final(&hashCtx, &hashOut); err != nil { return err; } return nil; }
- acdha 13y ago
  I would probably flag the code above in a security review because it hides the key part at the end of a complex line. Unless you're coding on VT100 terminal it's worth the extra line to make the test logic incredibly obvious: err = SSLHashSHA1.update(&hashCtx, &serverRandom); if (err != nil) { return err; }
  - voidlogic 13y ago
    In actual code things would not be named as they were above and it would be shorter, I was just trying to make it look reasonably like the C for HN.
    - acdha 13y ago
      True, but I've definitely noticed that particular style of writing if tests using a one-line assignment and obscured test condition seems to be pretty common in the Go community and it's a bad habit for understanding code.
      - voidlogic 13y ago
        >>it's a bad habit for understanding code. Is there objective evidence for this? As a Go programmer a semicolon in an if statement screams to me. I can see it possibly being in issue for new Go programmers- but I don't remember it being one for me.
        acdha 13y ago
        I didn't do a survey but I remember that and frequently punting on error handling (`res, _ = something_which_could_error()`) showing up enough in the projects I saw on Github to stand out as a trend when I was writing a few first programs. I certainly hope that's just sampling error.
- AYBABTME 13y ago
  The `else if` is useless here, since you return anyway. It only adds noise. I rarely use the one-line `if err := ...; err !=nil ` idiom because its quite a mouthful. However when I do, I try to make sure it's not too much to grasp at once. Here the extra `else` goes against that. Alright, I know this is just a quick snippet on HN and all, I just thought I'd mention it anyways. Maybe next time you actually write that in code you'll think about my point. ;)
brodo 13y ago
No need for that. Just return early. Plus one should split up this gigantic function into several smaller ones.

Couldn't you just set a 'failed' boolean and wrap the fail: statements in a conditional check on it? Not that I'm saying all uses of goto, particularly this one, are necessarily absolute evil.

voidlogic 13y ago

I agree, I was thinking about what this situation would look like in other langs and when I turned to Go, I realized: While Go has goto for tricky situations like this, because it has defer you don't have to use it often, assuming the free calls were needed (and the vars were not going to be GC'd): defer SSLFreeBuffer(&hashCtx) defer SSLFreeBuffer(&signedHashes) if err = SSLHashSHA1.update(&hashCtx, &serverRandom); err != nil { return err; } else if err = SSLHashSHA1.update(&hashCtx, &signedParams); err != nil { return err; } else if err = SSLHashSHA1.final(&hashCtx, &hashOut); err != nil { return err; } return nil; }

acdha 13y ago
I would probably flag the code above in a security review because it hides the key part at the end of a complex line. Unless you're coding on VT100 terminal it's worth the extra line to make the test logic incredibly obvious: err = SSLHashSHA1.update(&hashCtx, &serverRandom); if (err != nil) { return err; }
- voidlogic 13y ago
  In actual code things would not be named as they were above and it would be shorter, I was just trying to make it look reasonably like the C for HN.
  - acdha 13y ago
    True, but I've definitely noticed that particular style of writing if tests using a one-line assignment and obscured test condition seems to be pretty common in the Go community and it's a bad habit for understanding code.
    - voidlogic 13y ago
      >>it's a bad habit for understanding code. Is there objective evidence for this? As a Go programmer a semicolon in an if statement screams to me. I can see it possibly being in issue for new Go programmers- but I don't remember it being one for me.
      - acdha 13y ago
        I didn't do a survey but I remember that and frequently punting on error handling (`res, _ = something_which_could_error()`) showing up enough in the projects I saw on Github to stand out as a trend when I was writing a few first programs. I certainly hope that's just sampling error.
AYBABTME 13y ago
The `else if` is useless here, since you return anyway. It only adds noise. I rarely use the one-line `if err := ...; err !=nil ` idiom because its quite a mouthful. However when I do, I try to make sure it's not too much to grasp at once. Here the extra `else` goes against that. Alright, I know this is just a quick snippet on HN and all, I just thought I'd mention it anyways. Maybe next time you actually write that in code you'll think about my point. ;)

brodo 13y ago

No need for that. Just return early. Plus one should split up this gigantic function into several smaller ones.