code review 6248048: net/rpc: improve response reading logic

net/rpc: improve response reading logic

CL 5956051 introduced too many call != nil checks, so
attempt to improve this by splitting logic into three
distinct parts.

[snaury, 2012-05-25 05:03:29]

Hello r@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[r, 2012-05-26 17:17:26]

http://codereview.appspot.com/6248048/diff/4001/src/pkg/net/rpc/client.go
File src/pkg/net/rpc/client.go (right):

http://codereview.appspot.com/6248048/diff/4001/src/pkg/net/rpc/client.go#new...
src/pkg/net/rpc/client.go:130: } else {
not sure this is correct. if call == nil, can't we get here with response.Error
set?

in any case the logic here is tortuous and could use a rethink. call != nil is
checked 3 times in 20 lines.

[snaury, 2012-05-26 18:57:00]

On 2012/05/26 17:17:26, r wrote:
> src/pkg/net/rpc/client.go:130: } else {
> not sure this is correct. if call == nil, can't we get here with
response.Error
> set?

If call == nil, then we'd get to the top part, and could never get to the
bottom, where we only end up if call != nil and response.Error == "". I must
have overthought it in my previous CL, I only realized it after you commited the
change and I looked at the diff with a fresh look.

> in any case the logic here is tortuous and could use a rethink. call != nil is
> checked 3 times in 20 lines.

My assumption is that "call != nil" check is supposed to be cheap. We could do
less checks by making it a 3 case, but it would be more code duplication. I'd do
it, but I'm not sure it's worth it, because top 2 cases would have the same
"read error body", and the bottom 2 cases would have the same "call.done". If
that's ok, I'll do it.

[snaury, 2012-05-26 19:24:37]

Hello r@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2012-05-26 19:56:05]

LGTM
this is much clearer.
now we can remove the elses by using continue, or else turn it into a switch.

[snaury, 2012-05-26 19:57:21]

Hello r@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[snaury, 2012-05-26 19:59:55]

On 2012/05/26 19:56:05, r wrote:
> LGTM
> this is much clearer.
> now we can remove the elses by using continue, or else turn it into a switch.

Ah, sorry, I fixed an error in a comment before I read your response. I'll look
into it.

[snaury, 2012-05-26 20:03:38]

Hello r@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2012-05-26 21:17:55]

LGTM
there's some duplication here, but splitting it up makes it harder to understand

[r, 2012-05-26 21:28:36]

*** Submitted as http://code.google.com/p/go/source/detail?r=050271fb599f ***

net/rpc: improve response reading logic

CL 5956051 introduced too many call != nil checks, so
attempt to improve this by splitting logic into three
distinct parts.

R=r
CC=golang-dev
http://codereview.appspot.com/6248048

Committer: Rob Pike <r@golang.org>