4
\$\begingroup\$

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
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Please include the description, or the important parts of it, within the question itself, not as a link. \$\endgroup\$ – Simon Forsberg Apr 17 '16 at 13:38
4
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ Unfortunately its my college work so: 1. I am not allowed to change package name 2. with start methods it is indeed redundant \$\endgroup\$ – Eugene Shymko Apr 17 '16 at 17:10
  • \$\begingroup\$ 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 \$\endgroup\$ – Eugene Shymko Apr 17 '16 at 17:19
  • \$\begingroup\$ If-else assignment can be done with a conditional ternary operator which I think is more readable than even the braced if. \$\endgroup\$ – CAD97 Apr 17 '16 at 17:45
0
\$\begingroup\$

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
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.