Java Multitasking (String concatenation)

Question

I would like to know if I did everything right and if there a way to do it more simple?

Task: Task link to html file

My main:

package zad2;


public class Main {
  public static void main(String[] args) throws InterruptedException {
    StringTask task = new StringTask("A", 70000);
    System.out.println("Task " + task.getState());
    task.start();
    if (args.length > 0 && args[0].equals("abort")) { 
    /*
       <- here add the code interrupting the task after a second 
          and run it in a separate thread 
    */
        Thread.sleep(500);
        task.abort();
        task.startNewTread();
    }
    while (!task.isDone()) {
      Thread.sleep(500);
      switch(task.getState()) {
        case RUNNING: System.out.print("R."); break;
        case ABORTED: System.out.println(" ... aborted."); break;
        case READY: System.out.println(" ... ready."); break;
        default: System.out.println("unknown state");
      }

    }
    System.out.println("Task " + task.getState());
    System.out.println(task.getResult().length());
  }
}

My StringTask:

package zad2;

public class StringTask implements Runnable {
    String line;
    String letter;
    int amount;
    boolean isAborted;
    TaskState stat;
    boolean wasAborted = false;

    public StringTask(String s, int i){
        line = "";
        letter = s;
        amount = i;
        stat = TaskState.CREATED;
        isAborted = false;
    }
    public StringTask(String s){
        this(s, 0);
    }
    @Override
    public void run() {
        while (amount > 0 && !isAborted){
            line += letter;
            amount--;
        }
        if(wasAborted == false) stat = TaskState.READY;
        else stat = TaskState.ABORTED;
    }

    public String getResult (){
        return line;
    }
    public TaskState getState (){
        return stat;
    }
    public void start (){
        isAborted = false;
        stat = TaskState.RUNNING;
        new Thread(this).start();
        //System.out.println("start");
        //run();
    }
    public void startNewTread (){
        isAborted = false;
        stat = TaskState.RUNNING;
        new Thread(this).start();
    }

    public void abort (){
        stat = TaskState.ABORTED;
        wasAborted = true;
        isAborted = true;
    }
    public boolean isDone(){
        if (stat == TaskState.READY || stat == TaskState.ABORTED) return true;
        return false;
        }
}

My TaskState:

package zad2;
public enum TaskState {
    CREATED, RUNNING, ABORTED, READY
}

Answer 1

JavaDoc, JavaDoc, JavaDoc.

---

package zad2;

That is not a good package name. It should ideally be:

package com.yourmailprovider.username.stringstuffy;

or

package com.yourwebsite.stringstruffy;

This makes it very easy to see from where the used classes are from.

---

task.start();
...
task.startNewTread();

Why are there two start methods?

---

public class StringTask implements Runnable

This is odd, your class implements Runnable but is not supposed to be used as a Runnable. Instead it has its own set of methods. I suggest that you do not implement Runnable and instead encapsulate the Runnable as private inner class.

---

String line;
String letter;
int amount;
boolean isAborted;
TaskState stat;
boolean wasAborted = false;

Why are all these variables package private and not private?

---

public StringTask(String s, int i)

These are very, very bad variable names. Here are two simple rules for variables naming:

1. Name a variable after what it contains or is doing.
2. You are only allowed to use one letter variable names for dimensions (x, y, z).

---

while (amount > 0 && !isAborted){
    line += letter;
    amount--;
}

What you want is a StringBuilder. A string concatenation is a very costly operation, everytime you concatenate a string a new String object has to be created (because String is immutable). The StringBuilder instead simply extends its internal buffer.

---

if(wasAborted == false) stat = TaskState.READY;
else stat = TaskState.ABORTED;

Oh dear, that is awful to read. I know that it looks more complicated, but you really should make a habit out of using braces for ifs:

if (!wasAborted) {
    stat = TaskState.READY;
} else {
    stat = TaskState.ABORTED;
}

---

public void start (){
    isAborted = false;
    stat = TaskState.RUNNING;
    new Thread(this).start();
    //System.out.println("start");
    //run();
}
public void startNewTread (){
    isAborted = false;
    stat = TaskState.RUNNING;
    new Thread(this).start();
}

Why are there two start methods again? They are even doing the same thing.

---

public boolean isDone(){
    if (stat == TaskState.READY || stat == TaskState.ABORTED) return true;
    return false;
    }

Now this is a prime example of confusing formatting.

---

CREATED, RUNNING, ABORTED, READY

What's the difference between READY and CREATED? Why do you care if the task has been aborted or not? Does not clean up its state at some point?

---

Why is there a separate boolean for if the task has been aborted?

Answer 2

I suggest to...

1. ... have a look at the state pattern: https://en.wikipedia.org/wiki/State_pattern. This is a perfect example for usage. Avoid enumerations for state representation in the sense of a state machine

2. ... think about the transitions and start conceptional with a state chart

3. ... avoid redundant and overlapping code fragments like booleans that can be derived from the TaskState itself
4. ... to keep the state CREATED as an indicator for what the name stands for and make the difference clear to the state READY