Create and organize some classes (hierarchy)

I'm trying to figure out what is the best way to create some objects in my application. The application is already running, but I want to structure things well and clean.

These objects represents paths and they are complicated (they receive various parameters in their constructors).

There are four types of classes that represents a particular path:

• Gpx

• Road

• TrackMarkers

• JoinedMarkers

In the application there can exist only one path (Gpx, TrackMarkers, JoinedMarkers) and (if the user wants to) a Road (Road class represents the road to the path). A Road can't exist alone.

My MainActivity must have access to the data of these classes once created. I did not create a hierarchy of these classes (or implemented a creational patterns like factory method, abstract factory, builder etc...) and these classes (which are similar) contains duplicated code (except for JoinedMarkers that is simpler).

All the code of these classes:

Gpx

```
public class Gpx {

/** lunghezza del tracciato gpx */
private float length;

/** elevazione media del tracciato gpx */
private double averageElevation;

/** lista di coordinate del tracciato */
private List coordinates;

/** file relativo a questo percorso gpx */
private File file;

/** contesto dell'activity principale */
private Context context;

public Gpx(File file, Context context) {
coordinates = new ArrayList();
this.file = file;
this.file = file;
build();
}

/**
* Esegue la costruzione del tracciato, inizializzando i campi privati
*/
private void build() {
ParserAsyncTask parserAsyncTask = new ParserAsyncTask();
parserAsyncTask.execute(file);
}

/**
* AsyncTask che effettua il parsing del file gpx
*/
private class ParserAsyncTask extends AsyncTask {

private ProgressDialog mDialog;

protected void onPreExecute() {
mDialog = ProgressDialog.show(context,

Duplicated code

You have indeed some duplicated code, sometimes just a little off.

You can make abstract class for MultipleTracksAsyncTask and PathToTrackAsyncTask and move the duplicated code to there.

I should move to an private method of the abstract class.

if(mDialog != null) {
    if(mDialog.isShowing()) {
        mDialog.dismiss();
    }
}

Same for this but I should make an abstract method and make the implementation later cause it's for the 2 classes a little different :

if(result != null) {
    parseJSON(result);
}

So the result in the abstract class could be (You know what exactly the code does so make the names better for your cause):

@Override
protected void onPostExecute(String result) {
    updateMDialog();
    updateResult();
}

The same for your basic classes.
I see lenght and context always popup(except for JoinedMarkers) and Road and TrackMarkers have even more in common.

Try abstracting this to an abstract class.

Dangerous situation

Now I guess you have overlooked it but you have a potential bug in your code.

In the class Gpx you have the List and calculates the distance in async task.

You don't provide a setter cause that will break your distance.
The fault is that you have this getter.

public List getCoordinates() {
    return coordinates;
}

What's wrong with it?

Look at following code :

gpx.getCoordinates().clear();

Your list will be empty and your lenght will have incorrect value according this new list.

Program defensive and return an unmodifiable list back so no one can change it with the getter.

public List getCoordinates() {
    return Collections.unmodifiableList(coordinates);
}

Watch out: I put it here to point it out.

If after init the coordinates will not change anymore make it there unmodifiable.

So you won't constantly making a new unmodifiable list when you call the getter.

StackExchange Code Review Q#52422, answer score: 7