11
\$\begingroup\$

I'm making a text simulation game in C# and I've made the calculations and all. But I know there's a better, faster and more efficient way of doing this.

Basically, the first calculation is that there's a 5% chance that there would be a 80-100 yard gain and so on.

public static class Program
{
    public static Random r = new Random();
    public static int gained;
    public static int yardsLeft = 100;
    public static int i = 0;
    public static int chance = r.Next(1, 101);

    public static void Main()
    {
        Console.BackgroundColor = ConsoleColor.Blue;
        Console.ForegroundColor = ConsoleColor.Yellow;
        Console.Clear();

        //Console.WriteLine(BuffaloBills.TeamName + " will be facing off against the Seattle Seahawks" + "\n");
        //Console.WriteLine("The Bills starting lineup is Quarterback " + BuffaloBills.QB + ", " + "Running Back " + BuffaloBills.RB + " and Wide Receiver " + BuffaloBills.WR + "\n");

        while (yardsLeft > 0)
        {
            int[] playResult = new int[999];
            playResult[i] = r.Next(1, 4);

            switch (playResult[i])
            {
                case 1:
                    Console.WriteLine(BuffaloBills.QB + " hands it off to " + BuffaloBills.RB + " for a gain of " + Calculations.Play() + " yards. \n");
                    yardsLeft -= gained;
                    i++;
                    break;
                case 2:
                    Console.WriteLine(BuffaloBills.QB + " passes it " + BuffaloBills.WR + " for a gain of " + Calculations.Play() + " yards. \n");
                    yardsLeft -= gained;
                    i++;
                    break;
                case 3:
                    Console.WriteLine(BuffaloBills.QB + " doesn't find anyone open so he rushes for a gain of " + Calculations.Play() + " yards. \n");
                    yardsLeft -= gained;
                    i++;
                    break;
            }
        }
        Console.ReadKey();
    }
}

public static int Play()
{
    if (Program.yardsLeft >= 80)
    {
        if (Program.chance >= 1 && Program.chance <= 5)
            Program.gained = Program.r.Next(80, 101);
        else if (Program.chance >= 6 && Program.chance <= 12)
            Program.gained = Program.r.Next(60, 80);
        else if (Program.chance >= 13 && Program.chance <= 24)
            Program.gained = Program.r.Next(20, 60);
        else if (Program.chance >= 25 && Program.chance <= 100)
            Program.gained = Program.r.Next(1, 15);
    }
    else if (Program.yardsLeft <= 79 && Program.yardsLeft >= 60)
    {
        if (Program.chance >= 1 && Program.chance <= 10)
            Program.gained = Program.r.Next(60, 80);
        else if (Program.chance >= 11 && Program.chance <= 30)
            Program.gained = Program.r.Next(20, 60);
        else if (Program.chance >= 31 && Program.chance <= 100)
            Program.gained = Program.r.Next(1, 20);
    }
    else if (Program.yardsLeft <= 59 && Program.yardsLeft >= 40)
    {
        if (Program.chance >= 1 && Program.chance <= 10)
            Program.gained = Program.r.Next(40, 60);
        else if (Program.chance >= 11 && Program.chance <= 30)
            Program.gained = Program.r.Next(20, 40);
        else if (Program.chance >= 31 && Program.chance <= 100)
            Program.gained = Program.r.Next(1, 20);
    }
    else if (Program.yardsLeft <= 39 && Program.yardsLeft >= 20)
    {
        if (Program.chance >= 1 && Program.chance <= 10)
            Program.gained = Program.r.Next(30, 40);
        else if (Program.chance >= 11 && Program.chance <= 30)
            Program.gained = Program.r.Next(10, 30);
        else if (Program.chance >= 31 && Program.chance <= 100)
            Program.gained = Program.r.Next(1, 10);
    }
    else if (Program.yardsLeft < 20)
    {
        if (Program.chance >= 1 && Program.chance <= 10)
            Program.gained = Program.r.Next(15, 20);
        if (Program.chance >= 11 && Program.chance <= 30)
            Program.gained = Program.r.Next(10, 15);
        if (Program.chance >= 31 && Program.chance <= 100)
            Program.gained = Program.r.Next(1, 10);

    }
    return Program.gained;
}
\$\endgroup\$
  • \$\begingroup\$ I'm guessing chance is some form of RNG? \$\endgroup\$ – Kaz Aug 31 '15 at 16:03
  • \$\begingroup\$ Yes. It's an american football text simulation game. So it generates a number of how many yards are progressed each play. \$\endgroup\$ – Donavon Aug 31 '15 at 16:05
  • \$\begingroup\$ I don't know american football so your numbers might be correct, but I'd like to remind you that the second argument in Random.Next is exclusive (not inclusive as the first argument). And BTW, how do you know there's a better, faster and more efficient way in doing this? ;) \$\endgroup\$ – Bjørn-Roger Kringsjå Aug 31 '15 at 16:14
  • \$\begingroup\$ @Bjørn-RogerKringsjå looking at the code, I'm pretty sure there's at least a more elegant, flexible and maintenance-friendly way to go about this. \$\endgroup\$ – Mathieu Guindon Aug 31 '15 at 16:17
  • 1
    \$\begingroup\$ @Donavon The value provided in the second argument (maxValue) will never be generated, it's exclusive. E.g. , if you want to generate a random number in the range 20-30 you need to invoke like this: r.Next(20, 31) or r.Next(20, (30 + 1)). This way it might generate the number 30 but not 31. \$\endgroup\$ – Bjørn-Roger Kringsjå Aug 31 '15 at 16:33
7
\$\begingroup\$

Since your step for the yardsLeft is exactly 20 you can replace outer ifs with the switch (yardsLeft / 20) operator.

And using the following helper methods:

  • Method to find index of occurrence within stacked ranges.

    private static int FindRange<T>(T x, params T[] rangeMarks)
        where T : IComparable<T>
    {
        for (int i = 0; i < rangeMarks.Length; i++)
            if (x.CompareTo(rangeMarks[i]) < 0)
                return i;
        return rangeMarks.Length;
    }
    
  • Method to generate gained value:

    private static int GetGained(int x, int[] rangeMarks, int[] gainedMarks)
    {
        int index = FindRange(x, rangeMarks);
        if (index == 0) // If x < rangeMarks[0]
        {
            return r.Next(gainedMarks[0], 101);
        }
        return index == rangeMarks.Length 
            ? r.Next(1, gainedMarks.Last())
            : r.Next(gainedMarks[index], gainedMarks[index - 1]);
    }
    

Your code turns into the following:

switch (yardsLeft / 20)
{
    case 0:
        gained = GetGained(chance, new[] { 0, 11, 31 }, new[] { 20, 15, 10 });
        break;
    case 1:
        gained = GetGained(chance, new[] { 0, 11, 31 }, new[] { 40, 30, 10 });
        break;
    case 2:
        gained = GetGained(chance, new[] { 0, 11, 31 }, new[] { 60, 40, 20 });
        break;
    case 3:
        gained = GetGained(chance, new[] { 0, 11, 31 }, new[] { 80, 60, 20 });
        break;
    default:
        gained = GetGained(chance, new[] { 6, 13, 25 }, new[] { 80, 60, 20 });
        break;
}
\$\endgroup\$
  • \$\begingroup\$ It gives me 0's for some reason on every outcome. \$\endgroup\$ – Donavon Aug 31 '15 at 22:04
  • \$\begingroup\$ @Donavon Possibly was yardsLeft >= 100. Fixed: case 4 replaced with default. \$\endgroup\$ – Dmitry Sep 1 '15 at 9:45
7
\$\begingroup\$

You could set up a static 2 dimensional array with pre-calculated values to pass to r.Next() for each yardsLeft and chance combination.

e.g. array[80,1] would contain int tuple {80, 101}

\$\endgroup\$
  • \$\begingroup\$ Welcome to CodeReview, John! Very sensible suggestion. \$\endgroup\$ – Caridorc Aug 31 '15 at 17:17
1
\$\begingroup\$

What if you want to move away from a console application and want to create a WPF application, or an ASP.NET application, etc.? You'd need to change so much code. Don't link your code so closely to the technology, instead move it to a dedicated class.

If public static int Play() was part of a dedicated class that handled all of the game play, you wouldn't need to have all those ugly Program. calls.

The logic also feels wrong: calling Play() updates gained, which is returned because you use it in Console.WriteLine(). Considering you call Calculations.Play() in each of the three cases, why not call it outside of the switch, don't have it return gained and simply use gained?

Also, considering that each of the cases is basically the same, except for the message, why not simply use the switch to write the message and move yardsLeft -= gained; and i++; outside of the switch?

Console.WriteLine() can be used in the same way as string.Format(), so avoid concatenation.

But again: these are minor fixes, the big fix is moving all that gameplay code to a specific class and only using Main() in Program for I/O.


public static Random r = new Random();
public static int gained;
public static int yardsLeft = 100;
public static int i = 0;
public static int chance = r.Next(1, 101);

I'm not fond of public fields, certainly not when they're badly named: r.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.