A VB convert...

J

Jibrohni

Freshman
Joined
Apr 15, 2010
Messages
28
Hey all, I'm just starting out using C# and am trying to learn by converting a program I've started in VB to C#.

Sadly I'm stuck early on with trying to generate an array of 52 randomly ordered integers. I have it working but fear that I'm not doing a tidy job. The routine seems very sluggish as opposed to it running in VB. Please can anyone advise on a better way to do this than my method below please?

I'm using a goto statement which I have a bad feeling about and it's also doing a fair few iterations that can most likely be avoided. Even so, it shouldn't be taking my nice new i5 processor 3-4 seconds??



Code: 
private int RandomNumber(int min, int max)
        {
            Random random = new Random();
            return random.Next(min, max);
        }
        public int[] GenerateDeck()
        {
            int[] DECK = new int[52];       //creates an array to hold thegenerated deck
            int iCounter = 1;

            do
            {
            Start:
                int rndNum = RandomNumber(0, 52);

                if (DECK[rndNum] == 0)
                {
                    DECK[rndNum] = iCounter;
                    iCounter++;
                }
                else
                    goto Start;
            } while (iCounter < 53);

            return DECK;

        }
private void btnGenerate_Click(object sender, EventArgs e)
        {

            GenerateDeck();

        }
I really appreciate your time. P.S. I like the efficiency and neatness of C# coding. Shame I'm crap at it...
 
I would start by using a for loop. These aren't too different between VB, the syntax is just painfully awkward and the terminating value is usually different by 1.
Code: 
For [COLOR="Blue"]I As Integer[/COLOR] = [COLOR="Green"]0[/COLOR] To [COLOR="Red"]9[/COLOR]
Next
Code: 
for([COLOR="Blue"]int i[/COLOR] = [COLOR="Green"]0[/COLOR]; i < [COLOR="Red"]10[/COLOR]; i++) { } // You could also change < 10 to <= 9

A for loop is the best way here. It makes it clear that you want to loop for 1 to 52.

Also, FYI, you can use the continue statement to go to the beginning of a loop. This could achieve the same effect as your goto, but we won't be needing this anyway.

So we could change your code to:
Code: 
   int[] DECK = new int[52];     [COLOR="Green"]  //creates an array to hold thegenerated deck[/COLOR]
       [COLOR="Red"] for(int iCounter = 1; iCounter < 53; iCounter++) {[/COLOR]
            
                int rndNum = RandomNumber(0, 52);

                if (DECK[rndNum] == 0)
                {
                    DECK[rndNum] = iCounter;
                    iCounter++;
                }
                else
                     [COLOR="Green"]// We'll be getting rid of this[/COLOR]
                    [COLOR="Red"]continue;
            } [/COLOR]

            return DECK;
Now, one problem you have here is that you keep picking random numbers until you find an empty spot. Theoretically, this could potentially take infinitely long. Realistically, it's just gonna take a long time.

The method I prefer is to know how many empty spots you have, and pick a random empty spot. For instance, if we have ten empty spots, pick a random number between 0 and 9. Then, you walk through the array counting how many empty spots you pass until you find the one you want.
Code: 
   int[] DECK = new int[52];       //creates an array to hold thegenerated deck
        [COLOR="Green"]// Loop over the values we will insert into the deck[/COLOR]
        for(int iCounter = 1; iCounter < 53; iCounter++) {

            [COLOR="Green"]// Determine the # of empty spots and pick a random one[/COLOR]
            int emptySpots = 53 - i;
            int insertIndex = RandomNumber(0, emptySpots);

            int currentLocation = 0; [COLOR="Green"]// Index within DECK[/COLOR]

            [COLOR="Green"]// Count through empty spots until we find the one we want[/COLOR]
            for(int emptySpotIndex = 0; emptySpotIndex <= insertIndex; emptySpotIndex++) {

                [COLOR="Green"]// Find the next empty spot[/COLOR]
                while(DECK[currentLocation] != 0) {[COLOR="Green"] // Go until we find a zero (empty)[/COLOR]
                    currentLocation++;
                }
            }

            [COLOR="Green"]// currentLocation now points to the nth empty spot, which is where we want to add a value.[/COLOR]
            DECK[currentLocation] = iCounter;
        }
        return DECK;

My computer is systematically failing one component at a time, and right now I'm limping along without C# installed, so I haven't tested this code.
 
Thanks for your time on this I really appreciate it.

This will work much better than the way I was grinding out the result.

I feel your pain too. I've just built a new upgraded pc trying to save money, HA! It's so far cost me £900 and I'm just about to buy a graphics card. (And this didn't include a screen). Oh well, I'll have to make sure I get my moneys worth...
 
Back
Top