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

My main:

package zad2;

public class Main {
public static void main(String[] args) throws InterruptedException {
if (args.length > 0 && args[0].equals("abort")) {
/*
and run it in a separate thread
*/
}
case RUNNING: System.out.print("R."); break;
case ABORTED: System.out.println(" ... aborted."); break;
default: System.out.println("unknown state");
}

}
}
}


package zad2;

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

line = "";
letter = s;
amount = i;
isAborted = false;
}
this(s, 0);
}
@Override
public void run() {
while (amount > 0 && !isAborted){
line += letter;
amount--;
}
}

public String getResult (){
return line;
}
return stat;
}
public void start (){
isAborted = false;
//System.out.println("start");
//run();
}
isAborted = false;
}

public void abort (){
wasAborted = true;
isAborted = true;
}
public boolean isDone(){
return false;
}
}


package zad2;
}

• Please include the description, or the important parts of it, within the question itself, not as a link. – Simon Forsberg Apr 17 '16 at 13:38

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();
...


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;
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;


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) {
} else {
}


public void start (){
isAborted = false;
//System.out.println("start");
//run();
}
isAborted = false;
}


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

public boolean isDone(){
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?

• Unfortunately its my college work so: 1. I am not allowed to change package name 2. with start methods it is indeed redundant – Eugene Shymko Apr 17 '16 at 17:10
• 3. implement Runnable is a demand 4. made variables private. My mistake 5. changed name of vars to understandable 6. I know about string builder but it's demand of the task again 7. condition indeed really confusing but i disagree with braces its just 2.5 times more lines without need 8. i really dont know how to make "isDone" method more readable, please help me with it. 9. CREATED is when object just created. READY is when the work is done.ABORTED is demand again. 10.Last bollean is becouse i need to know if it was aborted or not.Im not able to do it with enum cose it change after restart – Eugene Shymko Apr 17 '16 at 17:19
• If-else assignment can be done with a conditional ternary operator which I think is more readable than even the braced if. – CAD97 Apr 17 '16 at 17:45

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