Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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"};
}
}
share|improve this question

migrated from stackoverflow.com Aug 20 at 16:06

This question came from our site for professional and enthusiast programmers.

5  
I'd create RANDOM only once and use for all. –  huseyin tugrul buyukisik Aug 20 at 16:01
    
You should just make it return 'Orianna' all the time as she is clearly the master champion here. On a more serious, but off-topic, note - are you trying to make an Ultimate Bravery program? :) –  Dan Pantry Aug 20 at 21:22
    
@DanPantry I'm not trying to make an ultimate bravery program, this was purely just to choose champions for me when i cant decide what to play but it is specific to roles i.e top, mid etc. On a side note Teemo is not included in this for Obvious Reasons :) –  freebol96 Aug 21 at 8:16

4 Answers 4

up vote 12 down vote accepted

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)];
share|improve this answer
    
Please explain why the code in your answer is better, don't just dump it here. –  syb0rg Aug 20 at 16:10
1  
@syb0rg This is what I was doing, but the question suddenly migrated from SO. So it took a bit of time to get here :). –  AlexD Aug 20 at 16:14
1  
Much better! -(-1) == +1 And welcome to the site! –  syb0rg Aug 20 at 16:16
1  
Thanks! @AlexD This was very helpful and completely de-cluttered my code. I am fairly new to c# so thanks for explaining it :)))))). –  freebol96 Aug 20 at 16:16
1  
@TopinFrassi T.Next(a.Length) returns a value from 0 (inclusive) to a.Length (exclusive). So in your case the maximum index is 21. –  AlexD Aug 20 at 20:20

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 ;)

share|improve this answer
1  
I love overkill! If you are going to knock something out of the ball park, why not into the next state? That being said, what about using (champ => { return styles.Any(x => champ.Roles.Contains(x)); }); instead of your existing lambda expression? –  AWinkle Aug 20 at 20:01
    
This was the next idea for the program but i could not figure out how to do it, this is certainly very helpful +1. –  freebol96 Aug 21 at 9:51
1  
@AWinkle unfortunately I don't have VS handy right now. And the last time I really used good Lambdas was... right: never ;) --> Yea seems like my lambda could be replaced with that. –  Vogel612 Aug 21 at 10:40
    
@freebol96 you might be interested in the fact that I put this code up as a minimal working example on my github (see here). Do you want me to take it down, or is there a (github) repo of yours where I could contribute to this IMO quite useful project? –  Vogel612 Aug 21 at 10:41
    
@Vogel612 I don't have Github, this was just a simple little project that was to test using arrays as part of my learning c#, but i am glad to see that you think it is a great idea and you should carry on with developing it :) –  freebol96 Aug 21 at 10:46

You need to learn how to use a Dictionary

Currently I can see the current 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
share|improve this answer
    
yes, that seems a lot better, however the company that i am doing my apprenticeship at use visual studio 2005, and 'var' is not available to me. –  freebol96 Aug 20 at 16:37
1  
@freebol96 shouldn't be a problem, simply specify the types explicitly :) –  Mat's Mug Aug 20 at 16:39

In addition to what other wrote, I would say that it is better to use only one Random instance in the lifecycle 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, instanciate 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.

share|improve this answer

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.