3
\$\begingroup\$

I just felt a sting as for the first time, I copy-pasted code blocks, and made very little change to it, to overload it. It works fine, but I have 3 constructors that are basically copy-pasted. I'm hoping there is a smarter way to do this.

This simply creates a thread and runs it with 1, 2 or 3 arguments, and then prints numbers to System.out using that thread. I will include an example usage below. The code doesn't strike me as bad of itself, but the sheer repetition is really bugging me. All criticism welcome.

import java.lang.Thread;

/**
 * Generic class that creates a thread and then loops and prints numbers to stdout.
 */
public class NumLoopThread implements Runnable {
    Thread thread;
    private String threadName;
    private int sleepTime = 400;
    private int numLoops = 10;

    /** Default constructor with threadName */
    public NumLoopThread(String threadName) {
        this.threadName = threadName;
        thread = new Thread(this, threadName);
        thread.start();
    }
    /** Extra constructor with threadName and sleepTime */
    public NumLoopThread(int sleepTime, String threadName) {
        this.sleepTime = sleepTime;
        this.threadName = threadName;
        thread = new Thread(this, threadName);
        thread.start();
    }
    /** Extra constructor with threadName, sleepTime, and numberLoops */
    public NumLoopThread(int sleepTime, String threadName, int numberLoops) {
        this.sleepTime = sleepTime;
        this.threadName = threadName;
        this.numLoops = numberLoops;
        thread = new Thread(this, threadName);
        thread.start();
    }
    /** Run the thread from start() */
    public void run() {
        System.out.printf("%s starting%n", threadName);
        try {
            for(int i = 0; i < numLoops; i++) {
                Thread.sleep(sleepTime);
                System.out.printf("%s, i = %d%n", threadName, i);
            }
        } catch(InterruptedException exc) {
            exc.printStackTrace();
        }
        System.out.printf("%s ending%n", threadName);
    }
}

Example usage I wrote for an exercise (feel welcome to also criticize this, if you want). Looks like this paste after it's ran.

public class ThreadIsAlive {
    public static void main(String[] args) {
        System.out.println("Main thread starting");
        // create 3 running threads
        NumLoopThread t1 = new NumLoopThread(400, "t1");
        NumLoopThread t2 = new NumLoopThread(400, "t2");
        NumLoopThread t3 = new NumLoopThread(400, "t3");

        do {
            System.out.print(".");
            try {
                Thread.sleep(100);
            } catch (InterruptedException exc) {
                exc.printStackTrace();
            }
        //wait until all threads terminate:
        } while (t1.thread.isAlive() ||
                 t2.thread.isAlive() ||
                 t3.thread.isAlive());
        System.out.println("Main thread ending");
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ You really shouldn't be calling new Thread(this).start() from the ctor. Not only does that leak this in the ctor, it has the potential to cause nasty issues with deadlocking. Consider refactoring this to some sort of ThreadFactory type pattern. \$\endgroup\$ – Boris the Spider Apr 21 '16 at 9:40
11
\$\begingroup\$

Chained constructors.

I find it interesting that you described the single-parameter constructor as your "default" one, and the others as "extras". It's usually the other way around: the constructor that initializes all immutable fields is the "main" one, and any other constructor that works off default values would be "convenience" constructors.

public NumLoopThread(int sleepTime, String threadName, int numberLoops) {
    this.sleepTime = sleepTime;
    this.threadName = threadName;
    this.numLoops = numberLoops;
    thread = new Thread(this, threadName);
    thread.start();
}

Take out numberLoops, you get this one:

public NumLoopThread(int sleepTime, String threadName) {
    // pasta
}

Take out sleepTime, you get this one:

public NumLoopThread(String threadName) {
    // pasta
}

Looks like threadName should be the first parameter in all 3 constructors, for consistency:

public NumLoopThread(String threadName, int sleepTime) {
    // pasta
}
public NumLoopThread(String threadName, int sleepTime, int numberLoops) {
    this.sleepTime = sleepTime;
    this.threadName = threadName;
    this.numLoops = numberLoops;
    thread = new Thread(this, threadName);
    thread.start();
}

That way when you look at your overloads, they all look like they naturally extend each other, in some sort of chain.

You already have your defaults:

private int sleepTime = 400;
private int numLoops = 10;

Consider making them static final, and single-purpose:

private static final int defaultSleepTime = 400;
private static final int defaultNumLoops = 10;

And since the actual fields are only ever assigned in a constructor, they should be marked final, too:

private final int sleepTime;
private final int numLoops;

Use this(); as the first instruction in the body, to "chain" the constructors:

public NumLoopThread(String threadName, int sleepTime) {
    this(threadName, sleepTime, defaultNumLoops);
}

public NumLoopThread(String threadName) {
    this(threadName, defaultSleepTime, defaultNumLoops);
}

Voilà, no more pasta!


That said I'm not sure about start()ing the thread in the constructor; it makes your run method a bit misleading. In general it is not a good idea to pass your 'this' reference to to a thread and start it during construction. The thread could then start with an unfinished object. Generally you should leave the starting of the thread to a separate method. (ref)

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ ++ great answer but... Did you mean DEFAULT_SLEEP_TIME? ;-) \$\endgroup\$ – Phrancis Apr 21 '16 at 3:57
  • \$\begingroup\$ WHATEVER_ROCKS_YOUR_BOAT - you're probably right... althought I'd think SHOUTING_SNAKE_CASE would be for const things, right? \$\endgroup\$ – Mathieu Guindon Apr 21 '16 at 4:00
  • 2
    \$\begingroup\$ final is a const ;-) try changing it later, the JVM will get real angry \$\endgroup\$ – Phrancis Apr 21 '16 at 4:11
  • \$\begingroup\$ Seconding what Phrancis said. In Java, there is no const keyword – it's replaced with final. Final public fields are constants and should therefore be SHOUTY_SNAKE_CASE. \$\endgroup\$ – Fund Monica's Lawsuit Apr 21 '16 at 12:56

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.