Skip to content

Conversation

@jdcormie
Copy link
Member

@jdcormie jdcormie commented Oct 28, 2025

Binder[Client/Server]Transport uses the following "single exit point" style/pattern of control flow all over the place:

if (inState(ExpectedState)) {
  if (!doStep1ThatCanFail()) {
    // error handling
  } else if (!doStep2ThatCanFail()) {
    // error handling
  } else if (...) {
    ...
  } else {
    // Happy case.
  }
}

This has the advantage of a single exit point which, in C-like languages has historically been useful for unconditional cleanup like releasing locks and closing resources. However, in Java with exceptions, unconditional cleanup has to happen in a finally block and so we don't really benefit. On the other hand the single exit point pattern creates a lot of nesting:

  • The happy case is always nested at least two levels deep in an else.
  • Inserting a new no-fail step in the sequence (say between 1 and 2) is awkward. You have to either create an empty error handling block or create a new "else" clause, increasing the indent/nesting level by one.

In this PR, I reduce nesting and make life easier for future PRs by converting all BinderTransport methods to use the guard clause pattern instead:
https://refactoring.com/catalog/replaceNestedConditionalWithGuardClauses.html
https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html

The existing if/else style of error handling is hard to read and hard to change.
https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html

This is a pure refactor -- No behavior change.
…e" pattern.

The existing if/else style of error handling is hard to read and hard to change:
https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html

This is a pure refactor -- No behavior change.
…lause" pattern.

The existing if/else style of error handling is hard to read and hard to change:
https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html

This is a pure refactor -- No behavior change.
…ern.

The existing if/else style of error handling is hard to read and hard to change:
https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html

This is a pure refactor -- No behavior change.
The existing if/else style of error handling is hard to read and hard to change:
https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html

This is a pure refactor -- No behavior change.
@jdcormie jdcormie requested a review from ejona86 October 29, 2025 16:59
@jdcormie jdcormie merged commit 8275580 into grpc:master Oct 29, 2025
17 checks passed
@jdcormie jdcormie deleted the stop-nesting branch October 29, 2025 17:26
AgraVator pushed a commit to AgraVator/grpc-java that referenced this pull request Nov 3, 2025
…2449)

`Binder[Client/Server]Transport` uses the following "single exit point"
style/pattern of control flow all over the place:
```
if (inState(ExpectedState)) {
  if (!doStep1ThatCanFail()) {
    // error handling
  } else if (!doStep2ThatCanFail()) {
    // error handling
  } else if (...) {
    ...
  } else {
    // Happy case.
  }
}
```

This has the advantage of a single exit point which, in C-like languages
has historically been useful for unconditional cleanup like releasing
locks and closing resources. However, in Java with exceptions,
unconditional cleanup has to happen in a finally block and so we don't
really benefit. On the other hand the single exit point pattern creates
a lot of nesting:
- The happy case is always nested at least two levels deep in an `else`.
- Inserting a new no-fail step in the sequence (say between 1 and 2) is
awkward. You have to either create an empty error handling block or
create a new "else" clause, increasing the indent/nesting level by one.

In this PR, I reduce nesting and make life easier for future PRs by
converting all BinderTransport methods to use the guard clause pattern
instead:

https://refactoring.com/catalog/replaceNestedConditionalWithGuardClauses.html

https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants