4
\$\begingroup\$

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<String,String> 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{
            gotoNextActivity();
        }
    }else{
        lblStatus.setText(getString(R.string.app_up_to_date));
        gotoNextActivity();
    }
}
\$\endgroup\$
5
\$\begingroup\$

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

  1. 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.

  2. 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<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;
    }
    

    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 {
        ...
    }
    
  3. 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<String, String> 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 {
        ...
    }
    
  4. 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<String, String> 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<String, String> availableUpdates, 
               final UpdateType updateType, final String updateKey) {
        final boolean shouldConfirm = 
            (updateInstallationMode == UpdateMode.CONFIRM);
        final boolean shouldInstall = shouldConfirm ? 
            confirmInstallUpdate(updateType) : true;
        if (shouldInstall) {
            downloadUpdate(updateType, availableUpdates.get(updateKey));
        }
    }
    
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks so much! What I loved is that you took it step by step explaining your thought process along the way. That's really helpful - Excellent Answer! \$\endgroup\$ – W.K.S Feb 17 '14 at 6:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.