Does my main class have too much code?

Question

For some reason i have the feeling that i should put all of my thread starts and joins in a for loop just to reduce the amount of code. I dunno if that will hurt readability on this class? Also is it normal for a main class to be initialize just to call one method? I only did this since I was getting an error regarding static vs non static.

import java.util.Scanner;
import java.util.concurrent.CountDownLatch;

public class Main {


    public static String[] table;//Table for the ingredients
    public static void main(String[] args) {
        //Gets the number of iterations from the user then maybe pass it to the agent who will wait/notifty until the loop is over
        Main main = new Main();
        main.runThreads();
    }

    public void runThreads(){
        int numofTests;
        Scanner in = new Scanner(System.in);
        System.out.print("Enter the number of iterations to be completed:");
        numofTests = Integer.parseInt(in.nextLine());///Gets the number of tests from the user
        Agent agent = new Agent(numofTests);
        Smoker Pat = new Smoker ("paper", "Pat");
        Smoker Tom = new Smoker ("tobacco", "Tom");
        Smoker Matt = new Smoker ("matches", "Matt");

        for (int i = 0; i < numofTests; i++){
            Thread thread1 = new Thread(Pat);
            Thread thread2 = new Thread(Tom);
            Thread thread3 = new Thread(Matt);
            Thread thread4 = new Thread(agent);
            thread1.start();
            thread2.start();
            thread3.start();
            thread4.start();
            try {
                thread1.join();
                thread2.join();
                thread3.join();
                thread4.join();
            } catch (InterruptedException e) {
                Thread.currentThread().interrupt();
            }
            Smoker.latch = new CountDownLatch(1);//Resets the countdown latch for the smokers
            System.out.println("*********************");
        }
        System.out.println("*********************");
        System.out.println("Pat has smoked a total of " + Pat.numOfSmokes);
        System.out.println("Matt has smoked a total of " + Matt.numOfSmokes);
        System.out.println("Tom has smoked a total of " + Tom.numOfSmokes);
    }

    }

Answer 1

I would add some structure. As a general statement I'd say that good structure(s) helps reduce code. Make collections (list, array, whatever) for each of Smokers and Threads.

I would also look into string formatting so you can loop through the Smoker collection and do your output. You do not want to hard code "Pat", "Mat", "Tom". The Smoker class has a Name property, use it!

I only did this since I was getting an error regarding static vs non static.

You gotta do what you gotta do. It's fine.

Answer 2

I will add some thoughts about the code. Some other things were already said by radarbob in the corresponding answer.

import java.util.Scanner;
import java.util.concurrent.CountDownLatch;

public class Main {
    public static String[] table;//Table for the ingredients

table is not used, but probably in some other code. If it is node used, you should just delete it.

    public static void main(String[] args) {
        //Gets the number of iterations from the user then maybe pass it to the agent who will wait/notifty until the loop is over
        Main main = new Main();
        main.runThreads();
    }

In this example, you could make the runThreads method static and just use runThreads().

    public void runThreads(){

As written above, it could be public static void runThreads() {

        int numofTests;

I suggest to not use abbreviations. So name it numberOfTests or numberOfRuns.

        Scanner in = new Scanner(System.in);

This could be discussed. I would name it scannerIn, so everyone knows while reading what this is and is not confused with System.in. But as I said, it could be discussed.

        System.out.print("Enter the number of iterations to be completed:");
        numofTests = Integer.parseInt(in.nextLine());///Gets the number of tests from the user

The comment does not help, because it just repeats the line of the code. You should remove it. And I think for your goal, a int numofTests = in.nextInt(); would be fine, shorter and more clear.

        Agent agent = new Agent(numofTests);
        Smoker Pat = new Smoker ("paper", "Pat");
        Smoker Tom = new Smoker ("tobacco", "Tom");
        Smoker Matt = new Smoker ("matches", "Matt");

I do not know what happens here exactly. It could be interesting to use a Collection like a List for this.

        for (int i = 0; i < numofTests; i++){
            Thread thread1 = new Thread(Pat);
            Thread thread2 = new Thread(Tom);
            Thread thread3 = new Thread(Matt);
            Thread thread4 = new Thread(agent);
            thread1.start();
            thread2.start();
            thread3.start();
            thread4.start();
            try {
                thread1.join();
                thread2.join();
                thread3.join();
                thread4.join();
            } catch (InterruptedException e) {
                Thread.currentThread().interrupt();
            }

With a list, this would be more compact. If you do not need the loop variable, you could make this more clear with a while loop:

while (numofTests > 0) {
    ...
    --numofTests;
}

.

            Smoker.latch = new CountDownLatch(1);//Resets the countdown latch for the smokers
            System.out.println("*********************");
        }

I do not understand this part of the code. As long as you do not make any interesting things inside the Smoker class, this has no effect. And even if you do, I would doubt the effect. The .join() methods will already end all threads. So the CountDownLatch will most probably do nothing here. If you do some things inside the Smoker class, you should provide some update method like resetCountDownLatch, the name should contain some description what you are doing inside the method (which I do not know, so the general name). Anyone reading this will wonder what is happening there. In general it is a good idea to keep your object internals inside your object and do not expose it to the outer world.

        System.out.println("*********************");
        System.out.println("Pat has smoked a total of " + Pat.numOfSmokes);
        System.out.println("Matt has smoked a total of " + Matt.numOfSmokes);
        System.out.println("Tom has smoked a total of " + Tom.numOfSmokes);
    }
    }

This looks like you update the static numOfSmokes variables inside the instantiated objects. If you do this in a multithreaded environment, be sure to take care about synchronization.