Best practices to create extended class

I have two class Task and Award which extends Task. I create this class objects according JSON data. At the moment I have one method that read data passed data and create Task. If I need Award I use method which convert Task to Award. I would like to know if I am following best practice when I want have one method to read data and create object from this data.

I know that convertTaskToAward could be moved to class Award.

I see three possible path here:

-
Create another method public Award createAward(JsonData json) and use it to create Award instead of converting Task to Award.

-
Replace public Task createTask(JsonData json) with public Award createAward(JsonData json) and then downcast Award to Task if needed.

-
Leave as it is right now but move public Award convertTaskToAward(Task taskForConvert) to class Award.

class Task

```
public class Task {
protected int id;
protected String description;
private Type type;
private String difficulty;
protected int repeatCount;
private int points;
protected int tempPoints;
private int money;
private int tempMoney;
protected int time;
protected int tempTime;
private ArrayList accessWords;
private ArrayList lettersCount;
protected TaskCallback delegate;
protected boolean isDone;
private BaseGameActivity activity;
private boolean obstacleRemoved;
protected boolean powerupUsed;
private int award;
private SharedPreferences pref;
protected String startTime;
private Date endTimeData;

protected Date startTimeData;
private PauseableTimerHandler timer;
private boolean isBlock;
private boolean isCurrent;

public Task(Type type, int id, String difficulty, int repeatCount, int points, int money, int time, ArrayList accessWords, String description, int award, String startTime, boolean isDone, ArrayList lettersCount, boolean isCurrent, BaseGameActivity activity, SharedPreferences pref, TaskCal

A Task and an Award seem like very different things. Part of the reason you are having trouble converting them is because they don't want to be the same thing. It is much more likely that a certain Task might provide a certain Award when the player completes the required action. Also, what happens when a Task might produce two different Awards? Do you then need to create another subclass of Task that does the same things for the new Award? It would be better to just give a Task the award it will return as an argument in the constructor.

In general, composition is often easier to work with than inheritance. Even in some cases with a clear is-a relationship exists.

EDIT based on your response:

It sounds like a Task and an Award might share a number of common methods, but are not intrinsically the same type of thing. This is a perfect example for an interface. This would allow you to write a UI that work generically with instances of this interface. From your vague description it might look like:

interface ICompletableThing {
  boolean isCompleted();
  String getTitle();
  IStyle getDisplayStyle();
}

It is possible that you might end up with a base implementation of ICompletableThing that both Task and Award inherit from that implements some shared code, but it does not appear that an Award is a type of Task.

To more directly answer your question. I think the best implementation would be to have one function that takes in json and produces an Award and one function that takes in json and produces a Task. Depending on how you are getting your json, you might then have function that returns something like Either (where you can get the resultant instance in a type-safe way).

Instance variables:

There are 26 instance variables. That is too many. It seems like you are using Task as just a large bag of state without much care for the organization. Just because the json you parse the data from is organized this way doesn't mean that your object model has to be the same.

Some of your variables are private and some of them are protected. It's not clear what decides what is accessible to a child class. For example, startTime is protected yet endTimeData is private. To make matters worse, the variables are not grouped by accessibility, so a reader has to be careful to note which fields are which. (There is a grouping, but it is not clear why these variables are different from the other ones.)

There are a number of tempXXX variables. A temp prefix generally means that the variable is temporary and not maintained state. Could these variables have a more descriptive name? Or maybe they belong in a different class to indicate that they are the values that can change during the Task lifetime.

Any of the instance variables you don't intend to change should be marked as final. This will prevent you from accidentally changing a value that should be constant. This is even more important when you have some variables that change and some that don't.

Decide if you are going to use this. or not and be consistent. In the constructor you leave off this. for the variables where the names are not being shadowed. However, in decreasePoints() the argument is named pPoints. Since there is no documentation to say what pPoints is, I can only assume that the extra p is there so it doesn't shadow the instance variable. My preference is to always prefix an instance variable with an underscore. This way you never have to worry about variable shadowing and it is fewer characters than this..

Separation of concerns:

public ArrayList getAccessWords(Boolean forSave);

Task should not care about what other code does with it's data. Have a class that is in charge of serializing your data and do not put it in your data class. Now everyone who wants to retrieve the access words has to pass an additional argument because some of the serialization formating is managed by the data class. The fact that insert a single String into a list should be an indication that this function is being misused.

Additionally, since you don't have a single method that produces the serialized format, some where else, you still need to write a method that pieces together each of these chunks for data.

Another reason to not put serialization code in your data object is because you might end op with multiple different formats. In fact, you already do! You have this format you use for saving the state and the json format you talk about parsing from. What happens if you also want to write out the json format?

Use interfaces:

It is very unlikely that anyone calling your getter cares that the result is actually an ArrayList, List would be sufficient. Since you declared that your method returns a specific List implementation, you had to create a new list that just copies one you already have.

return new ArrayList(Arrays.asList(stringAW.toString()));

could just be

StackExchange Code Review Q#55425, answer score: 4