Random name-picker with array

Question

I created a program that picks a champion for a role on the game League of Legends using arrays and random numbers.

Is there a quicker and more efficient way of doing this?

There is one form that contains this:

namespace Random_Champ_Picker_V2
{
public partial class Form1 : Form
{
    public Form1()
    {
        InitializeComponent();
    }

    internal void button1_Click( object sender, EventArgs e )
    {


        arrays array = new arrays();
        if( radioButton1.Checked == true )
        {
            Random T = new Random();
            int t = T.Next( 0, 57 );
            string toop = (string) array.top.GetValue( t );
            textBox1.Text = toop;
        }
        else if( radioButton2.Checked == true )
        {
            Random T = new Random();
            int t = T.Next( 0, 38 );
            string toop = (string) array.jung.GetValue( t );
            textBox1.Text = toop;
        }
        else if( radioButton3.Checked == true )
        {
            Random T = new Random();
            int t = T.Next( 0, 41 );
            string toop = (string) array.mid.GetValue( t );
            textBox1.Text = toop;
        }
        else if( radioButton4.Checked == true )
        {
            Random T = new Random();
            int t = T.Next( 0, 15 );
            string toop = (string) array.marksmen.GetValue( t );
            textBox1.Text = toop;
        }
        else if( radioButton5.Checked == true )
        {
            Random T = new Random();
            int t = T.Next( 0, 18 );
            string toop = (string) array.supp.GetValue( t );
            textBox1.Text = toop;
        }

    }

Class that contains the arrays (I removed most champ names for this post not to be cluttered):

namespace Random_Champ_Picker_V2
{
internal class arrays
{
    internal string[] supp = new string[]    {"Taric","Thresh"};

    internal string[] marksmen = new string[]{"Ezreal","Ashe"};

    internal string[] top = new string[]{"Zac","Aatrox"};

    internal string[] jung = new string[]{"Amumu","Jarvan IV"};


    internal string[] mid = new string[]{"karma","ziggs"};
}
}

Answer 1

First of all, you do not need to create an instance of Random class every time.

Then, you may use the ternary operator (the ?: operator) to select the array from which you want to take the element.

And, assuming that you have to take a random element form the array, you may use just a[T.Next(a.Length).

Random T = new Random();
. . .
string[] a = radioButton1.Checked? array.top:
             radioButton2.Checked? array.jung:
             radioButton3.Checked? array.mid:
             radioButton4.Checked? array.marksmen:
             radioButton5.Checked? array.supp:
             null;
if (a != null)
    textBox1.Text = a[T.Next(a.Length)];

Answer 2

I would like to propose a revisit of your design
Alas, what I am about to propose may be considered overkill. But as there is no kill like overkill, here goes:

What you have here is a classical Kind-to-Value mapping with multiple values and multiple kinds per value.

To make your code more OOP you might want to consider introducing a Champion-class as follows:

public class Champion
{
    public IEnumerable<PlayStyle> Roles { get; private set; }
    public string Name { get; private set; }

    public Champion (string name, IEnumerable<PlayStyle> designedRoles) 
    {
        //some error checking, e.g. both not empty or null
        Name = name;
        Roles = designedRoles;
    }
}

with a LaneType PlayStyle / Role enum. This allows you to have a Champion be acceptable for multiple lane-types (domain knowledge ftw.)

public enum PlayStyle
{
    MARKSMAN, TANK, SUPPORT, MAGE, ASSASSIN, FIGHTER 
    //possibly expandable for more granularity
}

Then you could dynamically generate your checkboxes for the PlayStyle-values you can obtain by calling Enum.GetValues(PlayStyle);

You'd then proceed to pack all your champs (with their properly assigned roles) into a List (well any IEnumerable) and then query them with something like that:

public partial class Form1 : Form 
{
     private List<Champion> myChampions;
     private Random rnd = new Random();

     public Form1() 
     {
         InitializeComponent();
         CreatePlayStyleCheckBoxes();
         FillChampionList(); // work some magic here ;)
     }

     internal void PickChampion(object sender, EventArgs e)
     {
        IEnumerable<PlayStyle> styles = FetchSelectedPlayStyles();
        IEnumerable<Champion> validForChoice = myChampions.Where
            (champ => { return styles.Any(x => champ.Roles.Contains(x)); });
            // Thanks to AWinkle for this absolutely cool lambda expression.
        Champion randomChoice =
               validForChoice.ElementAt(rnd.Next(0, validForChoice.Count()));
        //alert the user of his lucky pick ;)
        result.Text = randomChoice.Name;
     }
     //useful helper methods like: CreatePlayStyleCheckBoxes() and FillChampionList();
}

as an aside, I renamed your button1_Click() to: SelectChampion(), but that's just a suggestion ;)

Answer 3

You need to learn how to use a Dictionary

Currently I can see the following patterns in your code:

• radioButton is "connected" to a string[]
• radioButton is "connected" to an int

So I would do something like:

Dictionary<RadioButton, string[]> availableArrays;
Dictionary<RadioButton, int> maxValues;
...
var array = new arrays();
availableArrays.Add(radioButton1, array.top);
availableArrays.Add(radioButton2, array.jung);
...
maxValues.Add(radioButton1, 57);
maxValues.Add(radioButton2, 38);

Then I would loop through the keys in one of the dictionaries, find the checkbox that was checked and with that checkbox:

var options = availableArrays.Get(selectedCheckbox);
var maxRandom = maxValues.Get(selectedCheckbox);

And then you have all the data you need:

int t = T.Next(0, maxRandom);
string toop = (string) options.GetValue(t);
textBox1.Text = toop;

I strongly suspect however, that the maxRandom is actually the length of the array in which case you don't need the Dictionary<RadioButton, int> maxValues as you can get the length of the array using options.Length so:

int t = T.Next(0, options.Length);

---

I would recommend that you spend a few minutes reading your variable names. They are currently very cryptic. Even your RadioButtons can be renamed.

Here are some suggestions:

• radioButton1 --> radioTop
• Random T --> Random random
• internal class arrays --> internal class RandomNames
• textBox1 --> chosenName

Answer 4

In addition to what others have written, I would say that it is better to use only one Random instance in the life-cycle of your application so the results of the Next method are more... random!

private static readonly Random _random = new Random();

You should try to name your variable in a more comprehensive way. If I need to read your code to fix a bug and see only t and T variables, the work will be much more tough. Say randomInt instead of t and championName instead of toop.

Since your arrays is only a "collection" of arrays, you should make all of it static, to save the instanciation cost. And you should name it better too! (ChampionsArrays maybe?)

Since you know you have a string[] instead of an Array, you don't need to use the GetValue method, you can use the indexer (array[x])

Finally, I assume (don't hesitate to tell me if I'm wrong), that your numbers 57,38,41,15,18 are the number of elements in each array. Ex. your top array probably has 58 elements in it? If not, the following is wrong and I apologise.

You should (as proposed @Simon André), use a Dictionary<RadioButton,String[]>.

//Note that I use the arrays as if they were static, if you don't want to, instantiate arrays!
var dictionary = new Dictionary<RadioButton, string[]>();
dictionary.Add(radioButton1, arrays.supp);
dictionary.Add(radioButton2, arrays.marksmen);
dictionary.Add(radioButton3, arrays.top);
dictionary.Add(radioButton4, arrays.jung);
dictionary.Add(radioButton5, arrays.mid);

Then, when you need to know which is pressed, you can get the array and select a random value

string[] selectedArray = dictionary.Single(x => x.Key.Checked).Value;
int randomIndex = _random.Next( 0, selectedArray.Length);
string selectedChampion = selectedArray[randomIndex];
textBox1.Text = selectedChampion;

As you can note, I renamed the variabled and used the indexer instead of GetValue as I proposed, and use the _random value I created at the top of my answer.