4
\$\begingroup\$

I'm just starting to get along with threads in Java and I need some code review, about interfaces, classes, managing threads, correct writing, secure threads, exposed objects etc.

Horse-Race Implementation:

package HorseRace;

public interface FinishingLine {

    public void arrive(Horse h);
}

package HorseRace;

import java.util.Vector;

public class FinishingLineImpl implements FinishingLine {

    Vector<String> Ranking = new Vector<String>(10);

    public FinishingLineImpl(){}
    @Override
    public void arrive(Horse h) {
        // TODO Auto-generated method stub
        Ranking.add(h.getId());
    }

    public void print()
    {
        for (int i = 0; i < 10 ; i++)
        {
            System.out.println("Place " + (i + 1)+ " is : " + Ranking.get(i));
        }
    }
}

package HorseRace;

public interface Horse {

    public void run();
    public String getId();
}

package HorseRace;
import java.util.Random;

public class HorseImpl implements Horse,Runnable {

    int _distance;
    String _id;
    FinishingLine _f;

    public HorseImpl(String id, FinishingLine f)
    {
        _id = id;
        _distance = 0;
        _f = f;
    }
    @Override
    public void run() {
        // TODO Auto-generated method stub
        Random rand = new Random();

        int n = rand.nextInt(10) + 1;

        while (_distance < 100)
        {
            try
            {
                Thread.sleep(n);
            }
            catch (Exception e)
            {
                e.printStackTrace();
            }
            _distance += 1;
        }

            _f.arrive(this);
    }

    @Override
    public String getId() {
        // TODO Auto-generated method stud
        return _id;
    }
}

package HorseRace;

import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;

public class Main {

    public static void main(String[] args) throws InterruptedException {
        // TODO Auto-generated method stub

        ExecutorService e = Executors.newFixedThreadPool(10);
        FinishingLineImpl f = new FinishingLineImpl();

        for (int i = 0; i<10 ; i++)
        {
            System.out.println("Creating a horse with id = " + (i+1));
            String strI = Integer.toString(i+1);
            HorseImpl h = new HorseImpl(strI,f);
            e.execute(h);
        }

        e.shutdown();
        e.awaitTermination(60000, TimeUnit.SECONDS); 

        f.print();
    }
}
\$\endgroup\$
2
\$\begingroup\$
// package name should be lower case
package HorseRace;

public interface FinishingLine {

    public void arrive(Horse h);
}

package HorseRace;

import java.util.Vector;

public class FinishingLineImpl implements FinishingLine {

    // Vector is discouraged, unless you are using an ancient version of java
    // use Collections.synchronizedList(new ArrayList()) instead
    // variable should start with a lowercase letter, i.e. camelCase
    Vector<String> Ranking = new Vector<String>(10);

    public FinishingLineImpl(){}
    @Override
    public void arrive(Horse h) {
        // TODO Auto-generated method stub
        Ranking.add(h.getId());
    }

    // override toString instead
    public void print()
    {
        // magic numbers are evil; use ranking.size() instead
        for (int i = 0; i < 10 ; i++)
        {
            System.out.println("Place " + (i + 1)+ " is : " + Ranking.get(i));
        }
    }
}

package HorseRace;

public interface Horse {

    public void run();
    // change String to Object so you can use anything for the ID
    public String getId();
}

package HorseRace;
import java.util.Random;

public class HorseImpl implements Horse,Runnable {

    // see previous comment regarding camelCase
    int _distance;
    String _id;
    FinishingLine _f;

    public HorseImpl(String id, FinishingLine f)
    {
        _id = id;
        _distance = 0;
        _f = f;
    }
    @Override
    public void run() {
        // TODO Auto-generated method stub
        Random rand = new Random();

        int n = rand.nextInt(10) + 1;

        while (_distance < 100)
        {
            try
            {
                // why sleep? 
                // why not create and increment a timeElapsed instance variable?
                // preferrably an AtomicInteger
                // then calculate ranking after the race, e.g. sort by elapsed time
                Thread.sleep(n);
            }
            catch (Exception e)
            {
                e.printStackTrace();
                // break out?
            }
            _distance += 1;
        }

            _f.arrive(this);
    }

    @Override
    public String getId() {
        return _id;
    }
}

package HorseRace;

import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;

public class Main {

    public static void main(String[] args) throws InterruptedException {
        // I suggest calculating nThreads as cpu count * 2
        // @see Runtime.getRuntime().availableProcessors();
        // please use descriptive variable names
        ExecutorService e = Executors.newFixedThreadPool(10);
        FinishingLineImpl f = new FinishingLineImpl();

        // another magic number
        for (int i = 0; i<10 ; i++)
        {
            System.out.println("Creating a horse with id = " + (i+1));
            // see comment regarding id type
            String strI = Integer.toString(i+1);
            HorseImpl h = new HorseImpl(strI,f);
            e.execute(h);
        }

        e.shutdown();
        // 16 hours? really?
        e.awaitTermination(60000, TimeUnit.SECONDS); 

        // change to System.out.println(f);
        f.print();
    }
}
\$\endgroup\$
  • \$\begingroup\$ First of all, thank you for your answer. What do you mean by "change String to Object so you can use anything for the ID"? and why "calculating nThreads as cpu count *2"? thanks again. \$\endgroup\$ – user2491179 Dec 6 '13 at 20:11
  • \$\begingroup\$ I suggested changing the return type of getId to Object so that you can use any object as the ID (so you don't have to convert an int to a String). The benefit of calculating nThreads is the application will scale automatically. A computer might have 1 cpu or it might have 8. It should work optimally either way. \$\endgroup\$ – Tag Dec 6 '13 at 20:30

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.