Previous Lecture Complete and continue  

  Just Use Return Values!

I ran across a simple example of a typical trap that programmers fall into when adding “a little bit” to existing code. We start with a simple asynchronous task using a pretty straightforward framework: implement an “Async Task” interface and let the container take responsibility for the asynchronous part of the equation.

private void mySuperFantasticJobThatAbsolutelyNeedsToBeAsynchronous() {
  AsyncTask<Void, Void, String> task = new AsyncTask<Void, Void, String>() {
    @Override
    protected String doInBackground(Void... params) {
      // My super-fantastic job that absolutely needs to be asynchronous
      return result;
    }

    @Override
    protected void onPostExecute(String asyncTaskResult) {
      // Handle the result from doInBackground()
    }
  }
}

You’ll notice that I expect my asynchronous task to return a String and not need any parameters, so my AsyncTask turns Void into String.1 This means that doInBackground turns a collection of Void into String.2 No matter: I simply implement doInBackground() to return a String and all goes according to plan… until I need something more.

Programmers don’t like to disturb code when they add code, because they have a healthy fear of existing code.3 In this particular case, the programmer found their original code optimistic and needed to add an error message. No problem!

private void mySuperFantasticJobThatAbsolutelyNeedsToBeAsynchronous() {
  AsyncTask<Void, Void, String> task = new AsyncTask<Void, Void, String>() {
    @Override
    protected String doInBackground(Void... params) {
      String errorMessage = null;
      // My super-fantastic job that absolutely needs to be asynchronous
      // and that _might_ set a value for errorMessage...
      return result;
    }

    @Override
    protected void onPostExecute(String asyncTaskResult) {
      // Handle the result from doInBackground()
      if (asyncTaskResult != null) {
        // SUCCESS!
        // Do wonderful success-oriented things, like switching
        // to the next view.
      }
      else if (errorMessage != null) {
        // OOPS!
        // Do embarrassing error-oriented things, like pop up
        // the error message in a temporary view widget.
      }
    }
  }
}

Well… problem. Risk, anyway. Since the programmer needs to use the error message in onPostExecute(), clearly the programmer needs to elevate errorMessage into a field of the AsyncTask instance in order to write a value in doInBackground() and read that value in onPostExecute(). Done!

Ugh.

Two Return Values

This async task object does something risky: it treats two similar things differently. When I look at errorMessage I see an optional return value. Even better, this code (the actual code on which I’ve based this example) follows a common pattern: errorMessage is null when asyncTaskResult is not and the other way around. Now when I look at asyncTaskResult and errorMessage I see a union type, which the Haskell kids call Either.

In C, I would create a union:

union async_task_result {
  char task_result[1024];
  char error_message[1024];
} async_task_result;

In Haskell, I would find this easier:

data AsyncTaskResult = TaskResult | ErrorMessage
data TaskResult = String
data ErrorMessage = String

In Java, sadly, I need to use a tiny object.

class AsyncTaskResult {
  public final String taskResult;
  public final String errorMessage;

  public static AsyncTaskResult success(String taskResult) {
    return new AsyncTaskResult(taskResult, null);
  }

  public static AsyncTaskResult failure(String errorMessage) {
    return new AsyncTaskResult(null, errorMessage);
  }

  private AsyncTaskResult(String taskResult, String errorMessage) {
    this.taskResult = taskResult;
    this.errorMessage = errorMessage;
  }
}

If you’d like to suggest a better Java solution, I’m all ears. (Update 2017-07-03: I found something better in Java.)

The Strings Are a Coincidence

Please do not do something like this:

String taskResultOrErrorMessage = ....;

I know this looks tempting, and you might even want to justify it as removing duplication; however, this duplication is clearly coincidental. Modeling an error message as text, at least on the surface, looks quite natural, even though it carries the risk of Primitive Obsession. We wouldn’t, however, model a generic task result (really just “a thing”) as text—not on my team, at least. This task result happens to currently be modeled as text… for now. Mere coincidence and not essential duplication. This provides a great example of duplicate code that happens not to reflect duplication of concepts. It happens.

Now you see why I have modeled this as two variables, even though they happen to have the same type so far.

Back To Two Return Values

At any rate, I see the task result and the error message as part of the same return value for doInBackground(), and since Java doesn’t support Either or union types directly, I’m forced to introduce a little Result Object. By doing so, I turn an implicit dependency into an explicit one, because doInBackground() now returns an AsyncTaskResult and onPostExecute() accepts one. When I want to test onPostExecute(), I don’t have to jump through hoops to set up my AsyncTaskResult with the errorMessage I want. Lest you think this a trivial matter, you try it.

Why Does This Happen?

How do designs turn out this way? I point to one quirk of human nature as a potential cause: once we write something down, we believe it more. Programmers have the additional problem of not trusting code (even if they wrote it) and therefore not wanting to change existing code if they can avoid it. Programmers feel this so strongly that they volunteer to create dangerous implicit dependencies between functions, rather than simply send data from one function to the other through the natural, simple, clear mechanism of the pipeline, where the return value from the first function becomes the input parameter of the second. You know: programming.

I don’t say this to denigrate programmers, and particularly not the programmer whose code triggered this essay. I say this to emphasize that we don’t have to succumb to the (implied) fear in this design. We don’t have to let it fester. We mustn’t—that’s where legacy code comes from.

How To Proceed Safely

Assuming the typical legacy code situation, meaning low trust in the tests (if any), I’d do the following:

  1. Create a new AsyncTask class, changing the type signature from String to AsyncTaskResult (I’d choose a better name if I knew more about the purpose of the task!), copying the code from the old class into the new one.
  2. In the new doInBackground(), instead of writing a value to the field, I’d create the new return value with the corresponding values.
  3. In the new onPostExecute(), instead of reading the value of the field, I’d just read it directly from the input parameter.
  4. Flip the switch in each client of AsyncTask one by one and test everything until I felt satisfied that everything works.
  5. Remove the old, now obsolete, asynchronous task.

Of course, if I already had tests that I trusted, I could proceed more incrementally.

  1. Create a “union type” class that, for now, only wraps the “task result”, meaning the success value.
  2. Change the return type of doInBackground() and onPostExecute() at the same time to the new “union type” return value, even though I’m only using the “task result” part of it so far.
  3. Add a field to the “union type” for the error message.
  4. In doInBackground(), write the same “error message” value to both the field and the “union type” object.
  5. In onPostExecute(), read the “error message” value from the “union type” object instead of the field.
  6. Verify that nobody’s reading the “error message” field, then remove it.

I might use either approach, depending on my mood that day.

The Point

If it acts like a return value and you use it like a return value, then just use a return value!

The Full Example

I noticed this design risk in the following code, generously donated by a client of mine. (Hey, it happens occasionally.)

private void authenticateWithGoogle() {
  AsyncTask<Void, Void, String> task = new AsyncTask<Void, Void, String>() {
    String errorMessage = null;

    @Override
    protected String doInBackground(Void... params) {
      try {
        return GoogleAuthUtil.getToken(
          loginActivity,
          Plus.AccountApi.getAccountName(loginActivity.mGoogleApiClient),
          String.format("oauth2:%s", Scopes.PLUS_LOGIN));
      } catch (IOException networkOrServerError) {
        Log.e(TAG, "Error authenticating with Google: " + networkOrServerError);
        errorMessage = "Network error: " + networkOrServerError.getMessage();
      } catch (UserRecoverableAuthException recoverable) {
        Log.w(TAG, "Recoverable Google OAuth error: " + recoverable.toString());
        /* We probably need to ask for permissions, so start the intent if there is none pending */
        if (!loginActivity.mGoogleIntentInProgress) {
          loginActivity.mGoogleIntentInProgress = true;
          Intent recover = recoverable.getIntent();
          loginActivity.startActivityForResult(recover, LoginActivity.RC_GOOGLE_LOGIN);
        }
      } catch (GoogleAuthException reported) {
        /* The call is not ever expected to succeed assuming you have already verified that
        * Google Play services is installed. */
        Log.e(TAG, "Error authenticating with Google: " + reported.getMessage(), reported);
        errorMessage = "Error authenticating with Google: " + reported.getMessage();
      }

      return null;
    }

    @Override
    protected void onPostExecute(String oauthToken) {
      final boolean oauthSessionCreated = (oauthToken != null);

      // onOauthRequestCompleted(oauthToken)

      // Handler #1 for "OAuth Authentication Request Completed"
      notifyViewOfLoginResult(oauthToken, oauthSessionCreated);

      // Handler #2 for "OAuth Authentication Request Completed"
      notifyActivityOfLoginResult(oauthToken, oauthSessionCreated);
    }

    private void notifyViewOfLoginResult(String oauthToken, boolean oauthSessionCreated) {
      loginView.setLoginEnabled(oauthSessionCreated);
      loginView.hideProgress();
    }

    private void notifyActivityOfLoginResult(String oauthToken, boolean oauthSessionCreated) {
      Intent resultIntent = new Intent();
      if (oauthSessionCreated) {
        resultIntent.putExtra("oauth_token", oauthToken);
      } else {
        boolean thereIsAnActiveErrorMessage = errorMessage != null;
        if (thereIsAnActiveErrorMessage) {
          resultIntent.putExtra("error", errorMessage);
        }
      }

      loginActivity.setResult(LoginActivity.RC_GOOGLE_LOGIN, resultIntent);
      loginActivity.finish();
      }
    };

    task.execute();
  }
}

You might notice just how difficult it is to see that doInBackground() and onPostExecute() are coupled through errorMessage: if you don’t look inside notifyActivityOfLoginResult(), then you won’t see it. Implicit dependencies, such as Temporal coupling, carry risk. That risk becomes worse when the implicit dependencies become harder to detect. If you remain unconvinced, then try testing these methods in isolation. Or not in isolation. Either way, you’ll find it strangely annoying.

static class GoogleAuthenticationResponse {
  public final String oauthToken;
  public final String rejectionMessage;

  public GoogleAuthenticationResponse(String oauthToken, String rejectionMessage) {
    this.oauthToken = oauthToken;
    this.rejectionMessage = rejectionMessage;
  }

  public static GoogleAuthenticationResponse authenticated(String oauthToken) {
    return new GoogleAuthenticationResponse(oauthToken, null);
  }

  public static GoogleAuthenticationResponse rejected(String message) {
    return new GoogleAuthenticationResponse(null, message);
  }

  public boolean succeeded() {
    return oauthToken != null;
  }
}
private void authenticateWithGoogle() {
  AsyncTask<Void, Void, GoogleAuthenticationResponse> task = new AsyncTask<Void, Void, GoogleAuthenticationResponse>() {
    @Override
    protected GoogleAuthenticationResponse doInBackground(Void... params) {
      try {
        String oauthToken = GoogleAuthUtil.getToken(
          loginActivity,
          Plus.AccountApi.getAccountName(loginActivity.mGoogleApiClient),
          String.format("oauth2:%s", Scopes.PLUS_LOGIN));
        return GoogleAuthenticationResponse.authenticated(oauthToken);
      } catch (IOException networkOrServerError) {
        Log.e(TAG, "Error authenticating with Google: " + networkOrServerError);
        return GoogleAuthenticationResponse.rejected("Network error: " + networkOrServerError.getMessage());
      } catch (UserRecoverableAuthException recoverable) {
        Log.w(TAG, "Recoverable Google OAuth error: " + recoverable.toString());
        /* We probably need to ask for permissions, so start the intent if there is none pending */
        if (!loginActivity.mGoogleIntentInProgress) {
          loginActivity.mGoogleIntentInProgress = true;
          Intent recover = recoverable.getIntent();
          loginActivity.startActivityForResult(recover, LoginActivity.RC_GOOGLE_LOGIN);
        }
        // Uhh... Y U NO RETURN!?!!?
        return null;
      } catch (GoogleAuthException reported) {
        /* The call is not ever expected to succeed assuming you have already verified that
        * Google Play services is installed. */
        Log.e(TAG, "Error authenticating with Google: " + reported.getMessage(), reported);
        return GoogleAuthenticationResponse.rejected("Error authenticating with Google: " + reported.getMessage());
      }

      return null; // unless I have all the branches covered up there
    }

    @Override
    protected void onPostExecute(GoogleAuthenticationResponse authenticationResponse) {
      // REFACTOR Move this onto GoogleAuthenticationResponse as method
      final boolean oauthSessionCreated = (authenticationResponse.oauthToken != null);

      // onOauthRequestCompleted(oauthToken)

      // REFACTOR Replace conditionals with polymorphism here.
      // Handler #1 for "OAuth Authentication Request Completed"
      notifyViewOfLoginResult(authenticationResponse);

      // Handler #2 for "OAuth Authentication Request Completed"
      notifyActivityOfLoginResult(authenticationResponse);
    }

    private void notifyViewOfLoginResult(GoogleAuthenticationResponse authenticationResponse) {
      loginView.setLoginEnabled(authenticationResponse.succeeded());
      loginView.hideProgress();
    }

    private void notifyActivityOfLoginResult(GoogleAuthenticationResponse authenticationResponse) {
      Intent resultIntent = new Intent();
      if (authenticationResponse.succeeded()) {
        resultIntent.putExtra("oauth_token",  authenticationResponse.oauthToken);
      } else {
        // No need to check for error message, since that's in the contract
        // of GoogleAuthenticationResponse now!
        resultIntent.putExtra("error", authenticationResponse.errorMessage);
      }

      loginActivity.setResult(LoginActivity.RC_GOOGLE_LOGIN, resultIntent);
      loginActivity.finish();
      }
    };

    task.execute();
  }
}

I prefer where this design appears headed. You might not like the public final fields in GoogleAuthenticationResponse, but for such a simple struct-like class, I don’t mind. Later, when this class attracts more code, we have the option to encapsulate those fields.

Epilogue

A Better Solution in Java

If you’re working in Java 8 or later, then I have a better idea… maybe. (I know: with legacy code, you probably wouldn’t be able to use this, but there’s got to be a lot of Java 8 legacy code out there…)

Go to https://github.com/vavr-io/vavr (evidently that’s “Java” upside down, at least in some fonts—it used to be called “javaslang”) and use their Either type.

Here, you can replace GoogleAuthenticationResponse with Either<String, String>. Hm. Maybe not, since this re-exposes some Primitive Obsession. Perhaps we should consider introducing Whole Value classes for OAuth Token and Error Message.

Perhaps we can have the best of both worlds: let GoogleAuthenticationResponse wrap Either<String, String>, so that we have the benefit of the helpful name “Google Authentication Response” (and perhaps we can improve that name later) while also having the benefit of treating the value as an Either by giving the Google Authentication Response the method toEither().

static class GoogleAuthenticationResponse {
  // Either error message or oauth token.
  private final Either<String, String> responseAsEither;

  public GoogleAuthenticationResponse(Either<String, String> responseAsEither) {
    this.responseAsEither = responseAsEither;
  }

  public static GoogleAuthenticationResponse authenticated(String oauthToken) {
    return new GoogleAuthenticationResponse(Either.right(oauthToken));
  }

  public static GoogleAuthenticationResponse rejected(String message) {
    return new GoogleAuthenticationResponse(Either.left(message));
  }

  public Either<String, String> toEither() {
    return responseAsEither;
  }
  
  // Useful for clarity. Better than .toEither().isRight()
  public boolean succeeded() {
    return responseAsEither.isRight();
  }
}

// and then...
// inside authenticateWithGoogle()...

    @Override
    protected void onPostExecute(GoogleAuthenticationResponse authenticationResponse) {
      // REFACTOR Move this onto GoogleAuthenticationResponse as method
      final boolean oauthSessionCreated = authenticationResponse.succeeded();

      // onOauthRequestCompleted(oauthToken)

      // REFACTOR Replace conditionals with polymorphism here.
      // Handler #1 for "OAuth Authentication Request Completed"
      notifyViewOfLoginResult(authenticationResponse);

      // Handler #2 for "OAuth Authentication Request Completed"
      notifyActivityOfLoginResult(authenticationResponse);
    }

    private void notifyViewOfLoginResult(GoogleAuthenticationResponse authenticationResponse) {
      loginView.setLoginEnabled(authenticationResponse.succeeded());
      loginView.hideProgress();
    }

    private void notifyActivityOfLoginResult(GoogleAuthenticationResponse authenticationResponse) {
      Intent resultIntent = new Intent();

      // Nicer, no?
      authenticationResponse.toEither().bimap(
        (errorMessage) -> resultIntent.putExtra("error", errorMessage),
        (oauthToken) -> resultIntent.putExtra("oauth_token", oauthToken)
      );

      loginActivity.setResult(LoginActivity.RC_GOOGLE_LOGIN, resultIntent);
      loginActivity.finish();
      }
    };

I like it. I’d push for it. It takes a little getting used to, but it cleans the code up quite considerably.

(Yes, I know there’s other stuff to clean up here. That’s legacy code. I’ll get to it.)


  1. I don’t know what the second Void in the task’s type descriptor is about. Legacy code.

  2. No, I don’t know why it needs a collection of Void parameters. Again, legacy code. I imagine that it seemed like a good idea at the time.

  3. Maybe it’s not such a healthy fear, but I prefer to give programmers the benefit of the doubt in this case.

Try This At Work!

Take 15 minutes and look through your code for an instance of this problem. You have (exactly) two methods in a class, where on method writes to a field and the other one reads from that field. They might be the only two methods that touch that field.

Look at the name of the field. Consider naming it more precisely or completely. I bet you that the name you choose has some kind of temporal cue in it, like “last”, “recent”, or “current”. That’s a clue that the sequence of method calls matters. When the sequence of method calls matters, consider designing the code to make that sequence more obvious! You can use the refactoring I have proposed in this article to do exactly that. This is an example of Poka-yoke: design it to make it easy and obvious to use correctly.

If you decide to do this refactoring in your code, do it safely: take a snapshot or start a version control branch in your project, set a timer for 30 minutes, then refactor while adding the necessary tests. (You might find it much easier to make the change first, then test it, rather than trying to test-drive it.) You might even prefer test-driving a replacement, then wiring the old code to the new code. After 30 minutes, decide whether to keep it, throw it away, or keep going.