Best practices to create extended class

Question

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:

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

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

3. 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<String> accessWords;
 private ArrayList<Integer> 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<String> accessWords, String description, int award, String startTime, boolean isDone, ArrayList<Integer> lettersCount, boolean isCurrent, BaseGameActivity activity, SharedPreferences pref, TaskCallback delegate) {
 this.type = type;
 this.id = id;
 this.difficulty = difficulty;
 this.repeatCount = repeatCount;
 this.points = points;
 this.money = money;
 this.time = time;
 this.accessWords = accessWords;
 this.startTime = startTime;
 this.lettersCount = lettersCount;
 this.isDone = isDone;
 this.pref = pref;
 this.delegate = delegate;
 this.description = description;
 this.activity = activity;
 tempPoints = points;
 tempMoney = money;
 tempTime = time;
 this.award = award;
 this.isCurrent = isCurrent;
 }
 public String getStartTime() {
 return startTime;
 }
 public String getDifficulty() {
 return difficulty;
 }
 public int getId() {
 return id;
 }
 public int getAward() {
 return award;
 }
 public int getTempPoints() {
 return tempPoints;
 }
 public String getDescription() {
 return description;
 }
 public boolean isDone() {
 return isDone;
 }
 public Type getType() {
 return type;
 }
 public int getRepeatCount() {
 return repeatCount;
 }
 public int getPoints() {
 return points;
 }
 public int getMoney() {
 return money;
 }
 public int getTime() {
 return time;
 }
 public ArrayList<String> getAccessWords(Boolean forSave) {
 if (!forSave){
 return accessWords;
 } else {
 StringBuilder stringAW = new StringBuilder();
 for (String s:accessWords){
 stringAW.append(s).append("|");
 }
 return new ArrayList<String>(Arrays.asList(stringAW.toString()));
 }
 }
 public ArrayList<Integer> getLettersCount(boolean forSave) {
 if (!forSave){
 return lettersCount;
 } else {
 StringBuilder stringLetterCount = new StringBuilder();
 for (Integer i:lettersCount){
 stringLetterCount.append(i);
 }
 int tempInt = 0;
 if (stringLetterCount.toString() != ""){
 tempInt = Integer.parseInt(stringLetterCount.toString());
 }
 return new ArrayList<Integer>(Arrays.asList(tempInt));
 }
 }
 public void reset() {
 tempPoints = points;
 tempMoney = money;
 tempTime = time;
 isDone = false;
 isBlock = false;
 }
 public void decreasePoints(int pPoints) {
 if (accessWords.contains("CONTINUOUS")) {
 points -= pPoints;
 if (points <= 0) {
 decreaseRepeat();
 }
 } else if (difficulty.equals("SPECIAL")) {
 tempPoints -= pPoints;
 if (tempPoints <= 0 && type != Type.Mazes && !isBlock) {
 decreaseRepeat();
 isBlock = true;
 }
 } else if (type == Type.Mazes) {
 tempPoints -= pPoints;
 if (tempPoints > 0) {
 decreaseRepeat();
 isBlock = true;
 }
 } else {
 tempPoints -= pPoints;
 if (tempPoints <= 0) {
 decreaseRepeat();
 }
 }
 }
 public void decreaseMoney(int pMoney) {
 if (!accessWords.contains("SPEND")) {
 tempMoney -= pMoney;
 if (tempMoney <= 0) {
 decreaseRepeat();
 }
 } else {
 money -= pMoney;
 if (money <= 0) {
 decreaseRepeat();
 }
 }
 }
 public void mazeTaskDoneWith(int mistakes) {
 if (mistakes <= points) {
 decreaseRepeat();
 }
 }
 public void decreaseRepeat() {
 if (isDone) return;
 if (type != Type.Mazes) {
 repeatCount--;
 if (repeatCount <= 0) {
 delegate.onDone(this);
 }
 } else {
 repeatCount--;
 if ((repeatCount <= 0 && !accessWords.contains("SOLVE") || (repeatCount <= 0 && accessWords.contains("SOLVE")))) {
 delegate.onDone(this);
 }
 }
 }
 public void decreaseeTime() {
 if (accessWords.contains("CONTINUOUS")) {
 time--;
 if (time <= 0 && !isDone) {
 decreaseRepeat();
 }
 } else {
 tempTime--;
 Log.i("Award id:"+id,"[Temp]Second pass. "+tempTime+" to go.");
 if (tempTime <= 0 && !isDone) {
 decreaseRepeat();
 }
 }
 }
 public Task clone() throws CloneNotSupportedException {
 return new Task(this.type, id, difficulty, this.repeatCount, this.points, this.Money, this.time, this.accessWords, this.description, this.award, this.startTime, this.isDone, this.lettersCount, this.isCurrent, this.activity, this.pref, this.delegate);
 }
 public void done() {
 if(!isDone) {
 Log.i("Task", id + "| " + this.getClass() + " is done: " + description);
 isDone = true;
 }
 }
 public TaskCallback getDelegate() {
 return delegate;
 }
}

class Award

public class Award extends Task {
 private Integer oneShot;
 private String title;
 private Integer tempRepeatCount;
 private HashMap<String, Integer> wordsUsedInGame;
 private int tempOneShot;
 public Award(Type type, int id, int repeatCount, int points, int szyflings, int time, ArrayList<String> accessWords, String description, int award, String startTime, boolean isDone, ArrayList<Integer> lettersCount, BaseGameActivity activity, SharedPreferences pref, TaskCallback pDelegate) {
 super(type, id, "", repeatCount, points, szyflings, time, accessWords, description, award, startTime, isDone, lettersCount, false, activity, pref, pDelegate);
 tempRepeatCount = repeatCount;
 if (id == 7) wordsUsedInGame = new HashMap<String, Integer>();
 }
 public String getTitle() {
 return title;
 }
 public void setTitle(String title) {
 this.title = title;
 }
 public Integer getOneTime() {
 return oneShot;
 }
 public void setOneShot(Integer oneShot) {
 this.oneShot = oneShot;
 }
 @Override
 public void decreaseRepeat() {
 if (getAccessWords(false).contains("LETTER_GAME") || getAccessWords(false).contains("MAZE_GAME")) {
 tempRepeatCount--;
 if (tempRepeatCount <= 0) {
 isDone = true;
 delegate.onDone(this);
 }
 } else {
 repeatCount--;
 if (repeatCount <= 0) {
 isDone = true;
 delegate.onDone(this);
 }
 }
 }
 @Override
 public void reset() {
 tempRepeatCount = repeatCount;
 switch (id) {
 case 7:
 wordsUsedInGame.clear();
 break;
 case 36:
 tempTime = time;
 break;
 default:
 super.reset();
 break;
 }
 }
 public void snapDate() {
 DateFormat dateFormat = new SimpleDateFormat("HH:mm dd/MM/yyyy");
 startTimeData = new Date();
 startTime = dateFormat.format(startTimeData);
 }
 public void checkDate() {
 if (!startTime.equals("")) {
 try {
 DateFormat dateFormat = new SimpleDateFormat("HH:mm dd/MM/yyyy");
 Date currentDate = new Date();
 startTimeData = dateFormat.parse(startTime);
 float timePassed = currentDate.getTime() - startTimeData.getTime();
 timePassed = ((timePassed / 1000) / 3600);
 Log.i("Award id:" + id, "timePassed");
 if (timePassed <= 1) decreaseRepeat();
 } catch (ParseException e) {
 e.printStackTrace();
 }
 }
 }
 @Override
 public void decreasePoints(int pPoints) {
 if (isDone) return;
 switch (id) {
 case 10:
 tempPoints -= pPoints;
 if (tempTime > 0 && tempPoints <= 0) {
 decreaseRepeat();
 }
 break;
 default:
 super.decreasePoints(pPoints);
 break;
 }
 }
 @Override
 public void decreaseeTime() {
 if (isDone) return;
 switch (id) {
 case 10:
 if (tempTime > 0) tempTime--;
 break;
 default:
 super.decreaseeTime();
 break;
 }
 }
}

Main class

class MainActivity {
 public void loadTaskAndAward(){
 Task task = createTask(jsonDataForTask);
 Award award = convertTaskToAward(createTask(jsonDataForAward));
 }
 public Task createTask(JsonData json){
 //..read description,time,money from json
 return new Task(dscription,time,money);
 }
 public Award convertTaskToAward(Task taskForConvert){
 Award awardForReturn = new Award(title,taskForConvert.getDescription(),taskForConvert.getTime(),taskForConvert.getMoney());
 }
}

• kiwiron: Award does not sound like a descendant of Task. Can you enlighten us?
• Błażej: Task is used for short term task. 3 of 50 can be active at one time. Award are achievements they all are active. Also Award extends Task by title, diffrent way of display when they are done, diffrent way of completion. Basically I create class Award to separate code for short term tasks and long term.

Answer 1

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<Task, Award> (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<String> 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<String>(Arrays.asList(stringAW.toString()));

could just be

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

if your return type was not so restrictive.

---

Hard coded constant strings:

You should remove all of them. At the very least, you should replace them with defined constants. This will allow you to change the actual representation (the string value) everywhere it is used with a single edit. It will also remove the chance for subtle errors if you have a typo in on of the hard coded values.

However, it is much more likely that you don't want to be using String in the first place. It seems like you have a finite set of values you expect to be an access word. Yet you are using a type that can store infinitely many value. It would make more sense to use an enum to define all of the valid access words. Now it will not be possible to put an invalid String into your list of access words.

---

There are a number of cases where you check if the type is Maze and do a specific thing if this is or isn't the case. You are doing a similar thing with your access words. This kind of programing won't scale well. Every time your get a new type of access word, you have to edit your existing Task method to account for this new type of logic.

It would be better have your Task be passed a model that tells it what to do when decreasePoints() is called. That way, when another Type comes along, you just have to define a model for the new type and it will be much less likely that your changes will break what happens when a decreasePoints() is called for a Maze. (This is another example of composition.)

---

Some times you have extra blank lines at the beginning and end of your if blocks. These are not doing anything and can be removed.

• Błażej: Thanks for Your time, sounds I have busy weekend. Sounds GOOD :)

Answer 2

• Your class is way too big and does too many things. You should aim for separation of concerns: one class does one thing (and one method does one thing).

• I don't like the temp variables. Separate the description of the task from the evolution of the task.

• Everything that has to do with the timer, I would put in some other class. Similarly for everything that is related to the callback.

• I would never set the Activity as a class member. Maybe you can have a method that takes the Activity as argument, without storing it. But even better, all aspects of the tasks which have to do with the Activity should be put in some class, which will also contain the task.

• Instead of naming a variable delegate, stick to callback. I know what a delegate is from C#, but most Android programmers would be confused by that name.

• Don't ever use clone, but define a copy constructor instead. It is well known that they completely messed up cloning in Java and the advice from the last 15 years is to never use it.

• Declare your variables as List or Map, not ArrayList and HashMap.

• Błażej: Thank you but why List or Map is better than ArrayList and HashMap?
• toto2: Good question. I give this advice so much that I forgot to explain why. List and Map are interfaces and ArrayList and HashMap are implementations. You want to declare the variables using the interface type. In general, you want to declare the variables using the highest (most abstract) interface or super-class. That way, you can easily change the implementation for your variables. You don't have to change ArrayList to LinkedList everywhere if you used List, for example. Also it makes the code more readable when you stick to the most general interface or super-class.

Answer 3

An Award Is not a Task. You should only use inheritance if and only if the relation between the children its parent is "IS". A Student is always a person, and a lion is an animal all the times, so inheritance makes sense in these cases. And in general, favor composition over inheritance.

Object oriented is all about hiding implementation within abstract types. Therefore, use List<> for public and a concrete implementation like ArrayList<> in private.

 public List<String> getAccessWords(Boolean forSave)
 // instead of
 public ArrayList<String> getAccessWords(Boolean forSave)

It gives more flexibility, you might consider to migrate to a different List in the future, LinkedList for example. The same goes for you HashMap, use Map instead.

Your class screams the Builder pattern, you have too many parameters, and it's quite hard for the user of your class to know what parameter means what.

You can use an enum for your difficulty instead of a string, it's more type safe.

enum Difficulty{
LOW,MED,HIGH
}

You using java.util.Date and you should never touch this crap, use Joda API instead, java.util.Date is the worst API a human being have ever wrote.

Use Yoda conditions in if statements, and that is starting by the constant

 if (!startTime.equals("")) 

this if will throw a NullPointerExcpetion if startTime is null, you can invert your if instead

 if(!"".equals(startTime))

You dont need to provide setters for everything, it would great if you dont have any setters in your code, mutability is an evil.

• Simon Forsberg: Declaring by interface (List instead of ArrayList) is good for private methods as well, not only for the public ones.
• Sleiman Jneidi: @SimonAndréForsberg sure, but it's incredibly important for public ones.
• Simon Forsberg: Yes, absolutely. I totally agree with that.