Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

    }
share|improve this question

2 Answers 2

up vote 7 down vote accepted

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.

share|improve this answer
    
Never heard of String foramting I'll have to look into it, if at all possible would you mind providing an example? –  Austin Davis Feb 24 '13 at 5:26
    
There are two ways: String.format() or System.out.printf(). Here is a nice reference for printf (applies to both): web.cerritos.edu/jwilson/SitePages/java_language_resources/…. You may also just want to override toString() for the smokers and just call System.out.println(smoker). –  E-Man Feb 24 '13 at 14:12
    
Awsome thank you –  Austin Davis Feb 25 '13 at 1:35

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.

share|improve this answer
    
Wow thanks for all the detailed comments. The table arrays is used in another class, and all the print with the numOfSmokes are called after the threads are finished. numOfSmokes isn't static so that each object has its own number. I'll definitely try and stop abbreviating my variables, its a bad habit –  Austin Davis Feb 27 '13 at 18:23
    
Ok, if it is not static, you should make it private and provide a getter. Everyone will expect this behavior (as well as myself in this example). And even if it could feel ridiculous for such small examples, it has a lot of advantages. So it is better to do it in the common way. –  tb- Feb 27 '13 at 19:09

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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