HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Optimizing Bingo Caller Method

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
calleroptimizingbingomethod

Problem

I am dong a competitive programming problem I have passed all the tests.
but my program times out. I need help optimizing my code so it can run faster.


Task:



-
Finish method: BingoCaller.GetNumber()


-
Return all numbers in the range of 1 until 75 once and in random order

-
If there are no numbers left, return an empty string

-
The numbers are returned one by one in Bingo style:


"I27", "N40", "B5", "B12", "I28", "O69", "B1", ...

-
These are the ranges that you must follow:



-
A number within range 1 to 15 starts with a 'B'

-
A number within range 16 to 30 starts with an 'I'

-
A number within range 31 to 45 starts with an 'N'

-
A number within range 46 to 60 starts with a 'G'

-
A number within range 61 to 75 starts with an 'O'


-
Use System.Random to generate your random numbers. Pass via the constructor for testing purposes.


My code below

```
using System;
using System.Collections.Generic;

public class BingoCaller
{

private Random random;

static List List = new List();

public BingoCaller(Random random)
{
this.random = random;

}

public string GetNumber()
{

Start:

var item = random.Next(75);

if (List.Contains(item))
{
goto Start;
}
else
{
List.Add(item);

if (item >= 61)
{
return "O" + item;
}
else if (item >= 46)
{
return "G" + item;
}
else if (item >= 31)
{
return "N" + item;
}
else if (item >= 16)
{

Solution

It is considered bad practice to use goto-statements in modern programming because it quickly gets hard to follow the structure, so avoid that. Instead you could use a while-loop in a kind of this manner:

while (list.Count < 75)
{
  var number = random.Next(75);
  if (list.Contains(number))
    continue;
    ...
}


var item = random.Next(75);


This statement sets item to a number in the interval of \$[0, 75[\$, but you were supposed to handle the interval \$[1, 75]\$

instead of if (item >= 61) it is more readable to write if (item > 60)

It seems that GetNumber() is missing a stop-condition. It will run forever between goto Start and Start: when the list is full. (That may be your "time out"-problem).

Instead of the long list of if-statements, you could use the simple relation between the prefix's position in the word "BINGO" and the random number to extract it as:

string bingo = "BINGO"; // Make it a class field.
int index = ...
char prefix = bingo[index]


(it has something to do with 15).

The pattern of filling a list with found numbers tends to be slower and slower in response time as the size of the list grows because the next random number is more and more likely to be in the list. Potentially a valid number may never be found...

A better approach could be to create the list of the 75 numbers in the constructor in advance and then remove them one by one from the list when found by random.Next():

...
var number = list[random.Next(0, list.Count)];
list.Remove(number);
...


This will decrease in response time and you avoid the looping.

A solution could be:

public class BingoCaller
  {
    private Random random;
    List list;
    string bingo = "BINGO";

    public BingoCaller(Random random)
    {
      this.random = random;
      list = Enumerable.Range(1, 75).ToList();
    }

    public string GetNumber()
    {
      if (list.Count == 0)
        return "";

      var number = list[random.Next(0, list.Count)];
      list.Remove(number);
      char prefix = bingo[(number - 1) / 15];
      return $"{prefix}{number}";
    }
  }

Code Snippets

while (list.Count < 75)
{
  var number = random.Next(75);
  if (list.Contains(number))
    continue;
    ...
}
var item = random.Next(75);
string bingo = "BINGO"; // Make it a class field.
int index = ...
char prefix = bingo[index]
...
var number = list[random.Next(0, list.Count)];
list.Remove(number);
...
public class BingoCaller
  {
    private Random random;
    List<int> list;
    string bingo = "BINGO";

    public BingoCaller(Random random)
    {
      this.random = random;
      list = Enumerable.Range(1, 75).ToList();
    }

    public string GetNumber()
    {
      if (list.Count == 0)
        return "";

      var number = list[random.Next(0, list.Count)];
      list.Remove(number);
      char prefix = bingo[(number - 1) / 15];
      return $"{prefix}{number}";
    }
  }

Context

StackExchange Code Review Q#155134, answer score: 7

Revisions (0)

No revisions yet.