8
\$\begingroup\$

I was working on a program that simulates the Buffon's Needle experiment. If anyone is not familiar with it, a Wiki link is provided - Buffons's Needle

The point of Buffon's experiment is to find the value of pi, or at least approximate it. Usually you draw a couple of lines and then you scatter pins around the board in which you drew the lines in, the more pins the better. Then you take the number of pins crossing the lines and divide that by the number of pins you first had and multiply that by 4.

Below, I have provided the code for my program which is written in Java. Right now, it seems as if there are too many variables/methods. Could someone help refine it so it functions and looks better?

import java.util.Random;
import java.util.Scanner;
public class buffonNeedle {
    public static void main(String[] args) {
        Scanner in = new Scanner (System.in);
        System.out.println("How many darts do you want in each trial?");
        int darts = in.nextInt();
        System.out.println("How many trials do you want conducted?");
        int trials = in.nextInt();
        for(int i = 0; i<trials; i++) {
            xVal(darts);
            yVal(darts);
            hits(darts);
            System.out.println(piCalc(darts));
        }
    }
    public static double [] xVal (int darts){
        double [] xVal = new double[darts];
        Random random = new Random();
        for (int i = 0; i<darts; i++){
            xVal[i] = -1+random.nextDouble()*2;
        }
        return xVal;
    }
    public static double [] yVal (int darts){
        double [] yVal = new double[darts];
        Random random = new Random();
        for (int i = 0; i<darts; i++){
            yVal[i] = -1+random.nextDouble()*2;
        }
        return yVal;
    }
    public static int hits (int darts){
        int hits = 0;
        double [] xVal = xVal(darts);
        double [] yVal = yVal(darts);
        for(int i = 0; i<darts; i++){
            if((Math.pow(xVal[i],2)+Math.pow(yVal[i],2))<=1){
                hits++;
            }
        }
        return hits;
    }
    public static double piCalc(int darts){
        double hits = hits(darts);
        double pi = (hits/darts)* 4;
        return pi;
    }
}
\$\endgroup\$
5
\$\begingroup\$

In this loop…

    for(int i = 0; i<trials; i++) {
        xVal(darts);
        yVal(darts);
        hits(darts);
        System.out.println(piCalc(darts));
    }

You are throwing away the values returned from xVal(darts), yVal(darts), and hits(darts), making these calls completely superfluous. All they are doing is wasting entropy from the random number generator. The only thing that matters is piCalc(darts) (which calls hits(), which in turn calls xVal() and yVal()).

The code for xVal() is exactly identical to yVal(). You don't need to write the same function twice.

\$\endgroup\$
  • \$\begingroup\$ But, without it, the program ceases to run. \$\endgroup\$ – Vishvak Seenichamy Jul 20 '15 at 19:51
  • 1
    \$\begingroup\$ @VishvakSeenichamy Just replace all yVal with xVal in the program. \$\endgroup\$ – Caridorc Jul 21 '15 at 6:49
  • \$\begingroup\$ @Caridorc what do you want me to replace them with? \$\endgroup\$ – Vishvak Seenichamy Jul 21 '15 at 12:10
  • \$\begingroup\$ @VishvakSeenichamy I am not sure maybe coordinate ? \$\endgroup\$ – Caridorc Jul 21 '15 at 12:13
  • 1
    \$\begingroup\$ I took out the Xval Yval and hits from the main method and it still works really well! \$\endgroup\$ – Vishvak Seenichamy Jul 21 '15 at 15:37
5
\$\begingroup\$

Avoid assignement just before returning

public static double piCalc(int darts){
      double hits = hits(darts);
      double pi = (hits/darts)* 4;
      return pi;
}

Should become:

public static double piCalc(int darts){
      return ((double)hits(darts) / darts) * 4;
}

Whenever you see

var = expression
return var

Go for this simplification.

Long descriptive names

hits is better named is_crossing_line? (Java does not allow ? in names so you will have to remove it) and xVal should be named get_coordinate but I am not sure of the latter. As a rule of thumb if you have got a 5 or less letter name, think about something more descriptive.

\$\endgroup\$
  • \$\begingroup\$ Shouldn't it better be isCrossingLine and getCoordinate according to Java convention? And I don't agree 100% to "5 or less" (depending on how big your thumb is :-) The keyword is "descriptive". \$\endgroup\$ – GeroldBroser reinstates Monica Jul 21 '15 at 11:41
  • \$\begingroup\$ @GeroldBroser Sure it would be. \$\endgroup\$ – Caridorc Jul 21 '15 at 11:42
  • \$\begingroup\$ @GeroldBroser Sure in fact if a function performs little duty the name can be short, anyway especially with auto-completion I prefer longer names. \$\endgroup\$ – Caridorc Jul 21 '15 at 12:14
  • \$\begingroup\$ @DavidArno exactly, I lack experience with static typing. It is corrected now. \$\endgroup\$ – Caridorc Jul 22 '15 at 15:10
2
\$\begingroup\$

Why even create the xVal and yVal arrays? And why recreate a Random object every time?

You could take a different approach by calculating the data and determining if it was a hit all in one go. I also changed the function name from piCalc to calcPi so it reads a little better, and used the try-with-resources feature of Java 1.7 on Scanner.

import java.util.Random;
import java.util.Scanner;

public class BuffonNeedle
{
    public static void main(String[] args)
    {
        try (Scanner in = new Scanner(System.in))
        {
            System.out.println("How many darts do you want in each trial?");
            int darts = in.nextInt();
            System.out.println("How many trials do you want conducted?");
            int trials = in.nextInt();

            for (int i = 0; i < trials; i++)
            {
                System.out.println(calcPi(darts));
            }
        }
    }

    private static double calcPi(int darts)
    {
        Random random = new Random();

        int hits = 0;
        for (int i = 0; i < darts; i++)
        {
            double xVal = Math.pow(-1 + random.nextDouble() * 2, 2);
            double yVal = Math.pow(-1 + random.nextDouble() * 2, 2);
            if ((xVal + yVal) <= 1)
            {
                hits++;
            }
        }

        return 4.0 * hits / darts;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback, however I was given a set of rules to create the program and I was told to have separated methods for random number creation and then calculation \$\endgroup\$ – Vishvak Seenichamy Jul 21 '15 at 16:54
1
\$\begingroup\$
  • You're mixing spaces-around-operators style. Best practices is to use spaces for better readability.
  • It's also best practices to let statements like if, for, ... be followed by a space to distinguish them from method invocations.
\$\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.