1

I'l trying to generate a unique random number generator with the snippet of code from below, but it's not working. The IF section is suppose to test if it's the first random number generated, if it is, it's suppose to add the first random number to the ArrayList, if it's not the first random number, it's supposed to check if the random number is already in the ArrayList and if it's in the ArrayList it's suppose to MsgBox and generate a new unique random number that is not already in the ArrayList and add it to the ArrayList, but it's not doing any of those. Any help would be greatly appreciated.

Public Class Form1
    Dim r As New Random
    Dim dLowestVal As Integer = 1
    Dim dHighestVal As Integer = 26
    Dim dItemAmount As Integer = 1
    Dim RollCheck As New HashSet(Of Integer)

    Private Sub btnExit_Click(sender As Object, e As EventArgs) Handles btnExit.Click
        End
    End Sub

    Private Sub btnRollDice_Click(sender As Object, e As EventArgs) Handles btnRollDice.Click
        lblRandomNo.Text = r.Next(dLowestVal, dHighestVal)

        lblItemAmount.Text = dItemAmount

        If dItemAmount = 1 Then
            RollCheck.Add(Val(lblRandomNo.Text))
        ElseIf (RollCheck.Contains(Val(lblRandomNo.Text))) Then
            MsgBox("Already Exists")
            lblRandomNo.Text = r.Next(dLowestVal, dHighestVal)
            RollCheck.Add(Val(lblRandomNo.Text))
        End If

        dItemAmount = dItemAmount + 1

Thanks in advance.

Steve
  • 213,761
  • 22
  • 232
  • 286
user3289583
  • 25
  • 1
  • 7
  • `Dim dItemAmount as Integer = 1`, `dItemAmount = dItemAmount + 1` would make it 2 before it reaches `If dItemAmount = 1 Then`, would it not? Then your `ElseIf` would never trigger because `RollCheck` is presumably empty. Set a breakpoint, use the debugger. – ProgrammingLlama Aug 04 '18 at 12:25
  • And after the check with Contains you call again _r,Next_ but you could get again a number that is already in your array – Steve Aug 04 '18 at 12:26
  • ArraryLists are obsolete since a while back (not officially, but practice-wise). Use a [**`List(Of Integer)`**](https://msdn.microsoft.com/en-us/library/6sh2ey19(v=vs.110).aspx) instead, or even better: a [**`HashSet(Of Integer)`**](https://msdn.microsoft.com/en-us/library/bb359438(v=vs.110).aspx), which is faster and doesn't allow duplicates. In addition, the [**`HashSet.Add()` method**](https://msdn.microsoft.com/en-us/library/bb353005(v=vs.110).aspx) returns `False` if the item you're trying to add already exists. – Visual Vincent Aug 04 '18 at 12:29
  • There are a lot of problems with this code, to be honest: 1) `dItemAmount` will be 2 before you've even done anything. 2) Your `ElseIf` will only trigger if the number you generate has previously been generated and saved to the list. 3) As @Steve said, you could generate a number already in the list. 4) You have no way of generating a number after the first run, unless you hit on one that already exists in the list (so that it enters your `ElseIf`) and then generate a new one. – ProgrammingLlama Aug 04 '18 at 12:31
  • I've changed the code slightly from your advise, using HashSet(Of Integer) and moving dItemAmount after the IF statement. But it's still not working. – user3289583 Aug 04 '18 at 12:44
  • Side note: **Never ever** use `End` to exit the application. Instead, use `Application.exit()` or even better, simply `Close()` should be sufficient if it's the only open form. – 41686d6564 stands w. Palestine Aug 04 '18 at 13:03
  • 1
    the easier alternative is to shuffle an array, and take the values one by one from it https://stackoverflow.com/questions/254844/random-array-using-linq-and-c-sharp – Slai Aug 04 '18 at 13:12
  • Possible duplicate of [Random array using LINQ and C#](https://stackoverflow.com/questions/254844/random-array-using-linq-and-c-sharp) – Blackwood Aug 04 '18 at 13:18

2 Answers2

6

You could replace your whole method with this simple one

' This is globally declared at the top of your form
Dim values As New List(Of Integer)

' This is called when you construct your form
' It will store consecutive integers from 1 to 25 (25 elements)
values = Enumerable.Range(1, 25).ToList()

This is the method that extract an integer from your values that is not already used

Private Sub Roll()
    ' Get an index in the values list
    Dim v = r.Next(0, values.Count)

    ' insert the value at that index to your RollCheck HashSet
    RollCheck.Add(values(v))

    ' Remove the found value from the values list, so the next call 
    ' cannot retrieve it again. 
    values.Remove(values(v))
End Sub

And you can call it from the previous event handler in this way

Private Sub btnRollDice_Click(sender As Object, e As EventArgs) Handles btnRollDice.Click
  if values.Count = 0 Then
      MessageBox("No more roll available")
  else
      Roll()
  End Sub
End Sub
Steve
  • 213,761
  • 22
  • 232
  • 286
2

The point of the HashSet is that since it doesn't allow duplicates you can just check the return value of Add() to determine whether the number was successfully inserted or if it already exists in the list.

If you want to keep trying until it succeeds all you have to do is wrap it in a loop:

If dHighestVal - dLowestVal >= RollCheck.Count Then
    'If the above check passes all unique values are MOST LIKELY already in the list. Exit to avoid infinite loop.
    MessageBox.Show("List is full!")
    Return 'Do not continue.
End If

Dim Num As Integer = r.Next(dLowestVal, dHighestVal)

'Iterate until a unique number was generated.
While Not RollCheck.Add(Num)
    MessageBox.Show("Already exists!")
    Num = r.Next(dLowestVal, dHighestVal)
End While

lblRandomNo.Text = Num

An alternative way of writing the loop is: While RollCheck.Add(Num) = False.

Visual Vincent
  • 18,045
  • 5
  • 28
  • 75
  • Thank you Visual Vincent, the solution you offered worked exactly as I wanted. Thank you again. Appreciated it greatly. – user3289583 Aug 04 '18 at 23:55
  • @user3289583 : Glad I could help! Good luck with your project! – Visual Vincent Aug 05 '18 at 00:24
  • Will the highest value ever be generated with this code? When calling `r.Next`, the second parameter is _exclusive_ so you would need to use `dHighestVal + 1`. – Chris Dunaway Aug 06 '18 at 14:51
  • @ChrisDunaway : True, but it all comes down to how the OP's code works/how he expects it to work. I just adapted my solution to the original code. – Visual Vincent Aug 06 '18 at 15:28