Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I am interested in calculating odds of winning UK Lottery. The format is that 6 numbers from 1-59 are drawn. I am interested only (at this stage) in the odds of winning the jackpot (matching six balls). As an aside, I'm interested in the odds for a total balls count of 49, and 59, to see the change in chance of winning.

The mathematical formula for calculating the odds is (where 49 is total balls, 6 is number drawn:

\$\text{Odds of winning} = \dfrac{49!}{(6!*(49-6)!}\$

The main method of my code is to collect input from the user on parameters of the draw.

I have a class called DrawInfo to store information about the draw. I have a simple method to return the Factorial of a number.

I have a method to calculate the odds of winning the jackpot. This is currently all in the one class, as a small, simple app. I do appreciate that DrawInfo could live in its own class.

class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine("Enter the total number of balls in the draw: ");
            int totalBalls = int.Parse(Console.ReadLine());

            Console.WriteLine("enter the number of balls drawn: ");
            int ballsDrawn = int.Parse(Console.ReadLine());

            DrawInfo di = new DrawInfo(totalBalls, ballsDrawn);

            int totalWinOdds = FindJackpotWinningOdds(di);
            Console.WriteLine(String.Format("the odds are 1/{0:n0}", totalWinOdds));
            Console.ReadLine();          
        }

        static int FindJackpotWinningOdds(DrawInfo di)
        {
            BigInteger totalBallsFactorialSum = Factorial(di.TotalBalls);
            BigInteger ballsDrawnFactorialSum = Factorial(di.BallsDrawn);

            BigInteger JackpotWinningOdds = 0;
            JackpotWinningOdds = totalBallsFactorialSum / ((ballsDrawnFactorialSum * Factorial((di.TotalBalls - di.BallsDrawn))));
            return (int)JackpotWinningOdds;
        }

        static BigInteger Factorial(BigInteger i)
        {
            if (i <= 1)
            {
                return 1;
            }
            return i * Factorial(i - 1);
        }
    }

    public class DrawInfo
    {
        public int TotalBalls { get; set; }
        public int BallsDrawn { get; set; }

        public DrawInfo(int totalBalls, int ballsDrawn)
        {
            this.TotalBalls = totalBalls;
            this.BallsDrawn = ballsDrawn;
        }
    }
share|improve this question
up vote 6 down vote accepted

Quick remarks:

  • Don't abbreviate needlessly: di.
  • Why assign totalBallsFactorialSum and ballsDrawnFactorialSum, when you are only using them once? You're not even consistent: in the case of Factorial((di.TotalBalls - di.BallsDrawn)) you don't assign the result to a variable.
  • Don't overdo it with the brackets: there's no point for the inner ones in Factorial((di.TotalBalls - di.BallsDrawn)).
  • JackpotWinningOdds doesn't folow the capitalization conventions.
  • Why is it called FindJackpotWinningOdds? Wouldn't CalculateJackpotOdds be better?
  • The this in this.TotalBalls = totalBalls; and this.BallsDrawn = ballsDrawn; is superfluous.
  • TotalBalls and BallsDrawn should be private set;.
  • Why even assign the result to JackpotWinningOdds? This whole method can be reduced to a one-liner, though perhaps it would be best to split it over multiple lines to increase legibility:

        return (int)(
           Factorial(di.TotalBalls) 
           / (
              Factorial(di.BallsDrawn) 
              * Factorial(di.TotalBalls - di.BallsDrawn)
           )
        );
    

    This method could even just be a method on DrawInfo -- together with BigInteger Factorial(BigInteger i), of course, and Factorial() could then even be a private method.

share|improve this answer
    
Thanks. I was having problems calculating the 'JackpotWinningOdds' so was trying to break it down into individual variables to see where I was going wrong. Ideally I'd what you wrote above, as long as it's clear. – kafka Feb 1 at 15:36
    
assigning a meaningful name to a variable even if that variable is subsequently only used once is a very good way of documenting its semantic intent. – Alnitak Feb 1 at 18:23

A quick note to start:

Console.WriteLine(String.Format("the odds are 1/{0:n0}", totalWinOdds));

Is exactly the same as:

Console.WriteLine("the odds are 1/{0:n0}", totalWinOdds);

Now on to some maths fun... There's a multiplicative version which means you don't have to compute such massive numbers so don't need to use BigIntegers:

private int GetBinomialCoefficient(int totalNumberOfBalls, int numberOfBallsDrawn)
{
    // range checking of arguments omitted.
    var total = 1;
    for (var i = 1; i <= numberOfBallsDrawn; i++)
    {
        total *= (totalNumberOfBalls + 1 - i) / i;
    }
    return total;
}

GetBinomialCoefficient(59, 6) == 45057474

Edit

I have changed the name of the method away from GetJackpotOdds based on 200_success's excellent answer.

share|improve this answer
    
Thanks. I like the paths part of this answer, as I was wondering if there was a better way than having to work with massive numbers. – kafka Feb 1 at 16:06

You should fix your terminology.

The number of possible combinations is:

$$\left(\begin{array}{c}n\\r\end{array}\right) = \frac{n!}{r!\ (n-r)!}$$

The probability that any one ticket has the winning combination is:

$$\frac{1}{\left(\begin{array}{c}n\\r\end{array}\right)} = \frac{r!\ (n-r)!}{n!}$$

However, odds are a different convention for expressing probabilities. For example "1:1 odds" means a 50-50 chance; "1:3 odds" means a 25% chance. Therefore, the odds of winning the jackpot are

$$1\ :\ \frac{n!}{r!\ (n-r)!} - 1$$

In other words, you have an off-by-one error.


Calculating the result by actually computing \$49!\$ is nuts.

$$ \require{cancel} \begin{align} \frac{49!}{6!\ (49-6)!} - 1 &= \frac{49 \cdot 48 \cdot 47 \cdot \ldots \cdot 1}{(6 \cdot 5 \cdot 4 \cdot 3 \cdot 2 \cdot 1)(43 \cdot 42 \cdot \ldots \cdot 1)} - 1 \\ &= \frac{49 \cdot 48 \cdot 47 \cdot 46 \cdot 45 \cdot 44 \cdot \cancel{43} \cdot \cancel{42} \cdot \ldots \cdot \cancel{1}}{(6 \cdot 5 \cdot 4 \cdot 3 \cdot 2 \cdot 1)(\cancel{43} \cdot \cancel{42} \cdot \ldots \cdot \cancel{1})} - 1 \end{align} $$

That's 10 multiplications and one division. The numerator is a bit over 10 billion, which fits comfortably inside a C# long type.

share|improve this answer
    
Thanks. For clarify I'd rather just the probability of the winning (1 in 13,983,816 say) – kafka Feb 2 at 9:26

The chance the first ball matches an of your number is:

$$\frac{6}{49}$$

Assuming you hit this you have five numbers to match and 48 balls in the machine that can be picked.

So the second ball has a probability of:

$$\frac{5}{48}$$

Continue for all six balls picked.

You then have a probability of winning of:

$$= \frac{6}{49} \cdot \frac{5}{48} \cdot \frac{4}{47} \cdot \frac{3}{46} \cdot \frac{2}{45} \cdot \frac{1}{44}$$

$$= \frac{6 \cdot 5 \cdot 4 \cdot 3 \cdot 2 \cdot 1}{49 \cdot 48 \cdot 47 \cdot 46 \cdot 45 \cdot 44}$$

$$= \frac{720}{10,068,347,520}$$

$$= 1: 13,983,816$$

$$= 1 \space \text{in} \space 14 \space \text{million}$$

share|improve this answer
    
Thanks the maths here is what I have in my head, rather than what I found from Googling (and subsequently based my code on). – kafka Feb 2 at 9:27

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.