3
\$\begingroup\$

I have made an Android app called RunTracker - https://github.com/michaelnares/runtracker/tree/master/app. The intended functionality is that the app saves runs to the local filesystem, shows your current location and current run, and has a clickable list of runs. I feel that the existing code can be improved, but I'm not sure how. For example, the code for the MainActivity is quite verbose:

package com.michaelnares.runtracker.activities;

import android.arch.lifecycle.ViewModelProviders;
import android.content.Intent;
import android.support.v7.app.AppCompatActivity;
import android.os.Bundle;
import android.util.Log;
import android.view.View;
import android.view.WindowManager;
import android.widget.Button;
import android.widget.CompoundButton;
import android.widget.Switch;

import com.michaelnares.runtracker.R;
import com.michaelnares.runtracker.data.RunPoint;
import com.michaelnares.runtracker.utils.PermissionsHandler;
import com.michaelnares.runtracker.utils.RunsViewModel;
import com.michaelnares.runtracker.utils.Utils;

import org.json.JSONException;

import java.io.IOException;
import java.util.ArrayList;

public class MainActivity extends AppCompatActivity {

    private ArrayList<RunPoint> runPointsList = new ArrayList<>();
    private RunsViewModel runsViewModel;
    private AppCompatActivity activity = this;
    private PermissionsHandler permissionsHandler;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        permissionsHandler = new PermissionsHandler(this, MainActivity.this);
        setContentView(R.layout.activity_main);
        permissionsHandler.checkForPermissions();
        runsViewModel = ViewModelProviders.of(this).get(RunsViewModel.class);
        runsViewModel.setActivity(this);
        runsViewModel.setContext(getApplicationContext());

        final Button goToListButton = findViewById(R.id.goToList);
        goToListButton.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View v) {
                final Intent intent = new Intent(MainActivity.this, RunsListActivity.class);
                startActivity(intent);
            }
        }); // The setOnClickListener() block ends here.

        final Switch locationSwitch = findViewById(R.id.locationSwitch);
        locationSwitch.setChecked(runsViewModel.canTrackLocation());
        locationSwitch.setOnCheckedChangeListener(new CompoundButton.OnCheckedChangeListener() {
            @Override
            public void onCheckedChanged(CompoundButton buttonView, boolean isChecked) {
                runsViewModel.setCanTrackLocation(isChecked);
                if (isChecked && permissionsHandler.allConditionsMet()) {
                    permissionsHandler.startTrackingRuns();
                } else { // So save the current run, if the user switches the toggle off.
                    saveCurrentRun();
                }
            }
        });
        getWindow().addFlags(WindowManager.LayoutParams.FLAG_KEEP_SCREEN_ON); // Keeps the screen on always, when the app is running.
    } // The onCreate() method ends here


    private void saveCurrentRun() {

        if (runsViewModel == null) {
            runsViewModel = ViewModelProviders.of(this).get(RunsViewModel.class);
        }
        try {
            runsViewModel.saveCurrentRun(); // Saving them to external storage.
        } catch (JSONException e) {
            Log.e(Utils.getPackageName(MainActivity.this), "There was a JSONException whilst trying to save the journey points.");
            e.printStackTrace();
        } catch (IOException e) {
            Log.e(Utils.getPackageName(MainActivity.this), "There was an IOException whilst trying to save the journey points.");
            e.printStackTrace();
        }
    }
} // The main class ends here.

Can I rewrite the code to achieve better separation of concerns, i.e. less code in the MainActivity. How can the code be improved elsewhere?

\$\endgroup\$
1
  • \$\begingroup\$ How are this and MainActivity.this distinct in this context? There's no class nesting at your permissionsHandler assignment. \$\endgroup\$
    – Reinderien
    Commented Oct 2, 2019 at 14:06

1 Answer 1

3
\$\begingroup\$

I don't know anything about Android programming, so bear with me.

Unused variable

private AppCompatActivity activity = this;

This is unused - and even if it were used, you could just replace all references of it with this.

runPointsList is similarly unused.

Outer class this references

This code:

        @Override
        public void onClick(View v) {
            final Intent intent = new Intent(MainActivity.this, RunsListActivity.class);
            startActivity(intent);
        }

is justified in referencing MainActivity.this because it's an inner class. This code:

    permissionsHandler = new PermissionsHandler(this, MainActivity.this);

is not. You should be able to drop MainActivity. .

Multiple exceptions

This:

    } catch (JSONException e) {
        Log.e(Utils.getPackageName(MainActivity.this), "There was a JSONException whilst trying to save the journey points.");
        e.printStackTrace();
    } catch (IOException e) {
        Log.e(Utils.getPackageName(MainActivity.this), "There was an IOException whilst trying to save the journey points.");
        e.printStackTrace();
    }

should be able to have its two handlers collapsed to one with catch (JSONException|IOException e) syntax, since they do almost the same thing.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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