HiveBrain v1.2.0
Get Started
← Back to all entries
snippetjavaMinor

How can I reduce the number of nested if statements in this code?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thiscannumberthestatementsnestedreducehowcode

Problem

The application I'm building can accept two types of updates: Application Update and Parameter Updates. If there is an application update, then parameter updates can be ignored. This is the code that I've written, and it's messy. How can I clean this up?

```
mContext = this.getApplicationContext();
//experimental();
//check for updates
UpdateMode updateInstallationMode = Parameters.load(mContext).getUpdateMode();
if(updateInstallationMode==UpdateMode.MANUAL)
{
//User will manually install update - no further action needed to be performed by activity.
//proceed to next screen.
gotoNextActivity();
}else
{
boolean shouldConfirm = (updateInstallationMode == UpdateMode.CONFIRM);

lblStatus.setText(getString(R.string.checking_for_updates));
Error outUpdateError = new Error();
Map availableUpdates = checkForUpdates(outUpdateError);

boolean appUpdatesAvailable = availableUpdates.containsKey("application");
boolean paramUpdatesAvailable = availableUpdates.containsKey("parameters");
boolean appUpdatesAccepted = Parameters.load(mContext).getUpdateType()==UpdateType.BOTH
|| Parameters.load(mContext).getUpdateType()==UpdateType.APPLICATION;
boolean paramUpdatesAccepted = Parameters.load(mContext).getUpdateType() == UpdateType.BOTH
|| Parameters.load(mContext).getUpdateType() == UpdateType.PARAMETERS;

if(appUpdatesAccepted && appUpdatesAvailable){
boolean shouldInstall = shouldConfirm? confirmInstallUpdate(UpdateType.APPLICATION):true;
if(shouldInstall){
downloadUpdate(UpdateType.APPLICATION,availableUpdates.get("application"));
}else{
gotoNextActivity();
}

}else if(paramUpdatesAccepted && paramUpdatesAvailable){
boolean shouldInstall = shouldConfirm? confirmInstallUpdate(UpdateType.PARAMETERS):true;
if(shouldInstall){
downloadUpdate(UpdateType.PARAMETERS,availableUpdates.get("parameters"));
}else{

Solution

I assume that the code in the question is a method. If not extract it out to a method.

-

boolean appUpdatesAccepted = Parameters.load(mContext).getUpdateType() == UpdateType.BOTH
        || Parameters.load(mContext).getUpdateType() == UpdateType.APPLICATION;
boolean paramUpdatesAccepted = Parameters.load(mContext).getUpdateType() == UpdateType.BOTH
        || Parameters.load(mContext).getUpdateType() == UpdateType.PARAMETERS;


I'd start with removing some duplication here.

UpdateParameters updateParameters = Parameters.load(mContext);

...

final UpdateType updateType = updateParameters.getUpdateType();
boolean appUpdatesAccepted = updateType == UpdateType.BOTH
        || updateType == UpdateType.APPLICATION;
boolean paramUpdatesAccepted = updateType == UpdateType.BOTH
        || updateType == UpdateType.PARAMETERS;


I'm not familiar with Android, so UpdateParameters and UpdateType types could be something else.

-
If you reorder the boolean variables you can find some more duplication:

final boolean appUpdatesAvailable = availableUpdates
    .containsKey("application");
final boolean appUpdatesAccepted = updateType == UpdateType.BOTH
    || updateType == UpdateType.APPLICATION;

final boolean paramUpdatesAvailable = availableUpdates
    .containsKey("parameters");
final boolean paramUpdatesAccepted = updateType == UpdateType.BOTH
    || updateType == UpdateType.PARAMETERS;


This could be extracted out to a method:

private boolean isUpdateAvailable(
        final Map availableUpdates,
        final UpdateParameters updateParameters, 
        final UpdateType updateType, final String updateKey) {
    final UpdateType availableUpdateType = updateParameters
        .getUpdateType();
    final boolean appUpdatesAvailable = availableUpdates
        .containsKey(updateKey);
    final boolean appUpdatesAccepted = 
        availableUpdateType == UpdateType.BOTH 
        || availableUpdateType == updateType;
    return appUpdatesAvailable && appUpdatesAccepted;
}


Usage:

final boolean tryApplicationUpdate = 
    isUpdateAvailable(availableUpdates, updateParameters, 
        UpdateType.APPLICATION, "application");
final boolean tryParametersUpdate = 
    isUpdateAvailable(availableUpdates, updateParameters, 
        UpdateType.PARAMETERS, "parameters");
if (tryApplicationUpdate) {
    ...
} else if (tryParametersUpdate) {
    ...
} else {
    ...
}


-
It's also possible to extract out the first two if blocks to a method, they are very similar to each other:

private void downloadUpdateIfConfirmed(
        final boolean shouldConfirm, 
        final Map availableUpdates,
        final UpdateType currentUpdateType, final String updateKey) {
    final boolean shouldInstall = shouldConfirm ? 
        confirmInstallUpdate(currentUpdateType) : true;
    if (shouldInstall) {
        downloadUpdate(currentUpdateType, availableUpdates.get(updateKey));
    } else {
        gotoNextActivity();
    }
}


Usage:

if (tryApplicationUpdate) {
    downloadUpdateIfConfirmed(shouldConfirm, availableUpdates, 
        UpdateType.APPLICATION, "application");
} else if (tryParametersUpdate) {
    downloadUpdateIfConfirmed(shouldConfirm, availableUpdates, 
        UpdateType.PARAMETERS, "parameters");
} else {
    ...
}


-
I'd move the shouldConfirm flag into the method (to reduce its scope, see Effective Java, Second Edition, Item 45: Minimize the scope of local variables); use guard clauses; create another method which calls gotoNextActivity() to remove its duplication:

```
public void update() {
doUpdate();
gotoNextActivity();
}

private void doUpdate() {
mContext = this.getApplicationContext();
final UpdateParameters updateParameters = Parameters.load(mContext);
final UpdateMode updateInstallationMode =
updateParameters.getUpdateMode();
if (updateInstallationMode == UpdateMode.MANUAL) {
return;
}
lblStatus.setText(getString(RString.checking_for_updates));
final Error outUpdateError = new Error();
final Map availableUpdates =
checkForUpdates(outUpdateError);

final boolean tryApplicationUpdate =
isUpdateAvailable(availableUpdates, updateParameters,
UpdateType.APPLICATION, "application");
final boolean tryParametersUpdate =
isUpdateAvailable(availableUpdates, updateParameters,
UpdateType.PARAMETERS, "parameters");
if (tryApplicationUpdate) {
downloadUpdateIfConfirmed(updateInstallationMode,
availableUpdates, UpdateType.APPLICATION, "application");
return;
}
if (tryParametersUpdate) {
downloadUpdateIfConfirmed(updateInstallationMode,
availableUpdates, UpdateType.PARAMETERS, "parameters");
return;
}
lblStatus.setText(getString(RString.app_up_to_date));
}

private void downloadUpdateIfConfirmed(
final UpdateMode updateInstallationMode,
final Map availableUpd

Code Snippets

boolean appUpdatesAccepted = Parameters.load(mContext).getUpdateType() == UpdateType.BOTH
        || Parameters.load(mContext).getUpdateType() == UpdateType.APPLICATION;
boolean paramUpdatesAccepted = Parameters.load(mContext).getUpdateType() == UpdateType.BOTH
        || Parameters.load(mContext).getUpdateType() == UpdateType.PARAMETERS;
UpdateParameters updateParameters = Parameters.load(mContext);

...

final UpdateType updateType = updateParameters.getUpdateType();
boolean appUpdatesAccepted = updateType == UpdateType.BOTH
        || updateType == UpdateType.APPLICATION;
boolean paramUpdatesAccepted = updateType == UpdateType.BOTH
        || updateType == UpdateType.PARAMETERS;
final boolean appUpdatesAvailable = availableUpdates
    .containsKey("application");
final boolean appUpdatesAccepted = updateType == UpdateType.BOTH
    || updateType == UpdateType.APPLICATION;

final boolean paramUpdatesAvailable = availableUpdates
    .containsKey("parameters");
final boolean paramUpdatesAccepted = updateType == UpdateType.BOTH
    || updateType == UpdateType.PARAMETERS;
private boolean isUpdateAvailable(
        final Map<String, String> availableUpdates,
        final UpdateParameters updateParameters, 
        final UpdateType updateType, final String updateKey) {
    final UpdateType availableUpdateType = updateParameters
        .getUpdateType();
    final boolean appUpdatesAvailable = availableUpdates
        .containsKey(updateKey);
    final boolean appUpdatesAccepted = 
        availableUpdateType == UpdateType.BOTH 
        || availableUpdateType == updateType;
    return appUpdatesAvailable && appUpdatesAccepted;
}
final boolean tryApplicationUpdate = 
    isUpdateAvailable(availableUpdates, updateParameters, 
        UpdateType.APPLICATION, "application");
final boolean tryParametersUpdate = 
    isUpdateAvailable(availableUpdates, updateParameters, 
        UpdateType.PARAMETERS, "parameters");
if (tryApplicationUpdate) {
    ...
} else if (tryParametersUpdate) {
    ...
} else {
    ...
}

Context

StackExchange Code Review Q#41780, answer score: 5

Revisions (0)

No revisions yet.