search

soheilhy / cmux

Connection multiplexer for GoLang: serve different services on the same port!
Apache License 2.0
2.57k stars 196 forks source link

Use the readable indentation for error flow #21

Closed soheilhy closed 8 years ago

soheilhy commented 8 years ago

@shurcooL @tamird can you please review this pull request? thanks.

tamird commented 8 years ago

I don't find this more readable. Also, using berr as the variable name for an error is very likely to result in a typo - errors should almost always be named err to avoid confusion; this is asking for trouble.

:-1: from me.

soheilhy commented 8 years ago

well, actually there is no need for berr. that's not a shadow as it's in the same block. reverted that part.

tamird commented 8 years ago

ok, now you're overwriting err, so the semantics are changed. still :-1:

soheilhy commented 8 years ago

Yeah, but err isn't used really.

dmitshur commented 8 years ago

What's wrong with overwriting err? That one of the most common idioms in Go.

See https://gotools.org/net/http#request-go-L454-L479 for a very typical example from Go standard library.

You assign to err when doing some operation that may fail, and immediately check its value, responding appropriately (i.e., handle errors). Afterwards, you never look at that value of err again (it's already been handled), instead you reuse the variable for next similar operation.

In more rare cases where you care about persisting the error value for a longer time, you often use a variable name other than err.

This current change improves style and readability to me, while preserving behavior. LGTM from my side.

soheilhy commented 8 years ago

Then as per https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow, I'll merge this one. Sorry @tamird. :-)

tamird commented 8 years ago

What's wrong with overwriting err? That one of the most common idioms in Go.

There is nothing wrong with it, but it is being done here to avoid shadowing, not because it is the proper thing to do, and there is something wrong with that.