Random Message Generator using Arrays

```
using System;

namespace Friendly_Bot
{
internal class Program
{
private static string[] Greetings = { "Hello, you have a", "Hey, you have a", "Hi, you have a" };
private static string[] Compliments = { "cool", "fashionable", "stylish" };
private static string[] Garments = { "coat.", "dress.", "shirt.", "skirt.", "suit.", "swimsuit." };
private static string[] Farewells = { "ADIOS!", "BYE!", "BYE-BYE!", "FAREWELL!", "GOODBYE!" };

private static string Greeting;
private static string Compliment;
private static string Garment;
private static string Farewell;
private static string Message;

private static void Main(string[] args)
{
GenerateMessage();
}

private static void GenerateMessage()
{
int i = new Random().Next(1, 11);

//20% chance of including a farewell in the message.
if (i < 3)
{
Greeting = Greetings[new Random().Next(0, Greetings.Length)];
Compliment = Compliments[new Random().Next(0, Compliments.Length)];
Garment = Garments[new Random().Next(0, Garments.Length)];
Farewell = Farewells[new Random().Next(0, Farewells.Length)];
Message = Greeting + " " + Compliment + " " + Garment + " " + Farewell;
Console.WriteLine(Message);
Console.ReadLine();
}
//80% chance of NOT including a farewell in the message.
else
{
Greeting = Greetings[new Random().Next(0, Greetings.Length)];
Compliment = Compliments[new Random().Next(0, Compliments.Length)];
Garment = Garments[new Random().Next(0, Garments.Length)];
Message = Greeting + " " + Compliment + " " + Garment;
Console.WriteLine(Message);
Console.ReadLine();
}
}
}

-
The difference in the two branches of that if statement is just the Farwell so only this generation is what should happen inisde the if.

-
the name of the variable i is not choosen very well if you later on need a comment to describe what it should represent.

-
The GenerateMessage() method is doing more than its name implies. It is generating and outputting the message. This is also violating the single responsibility principle which means that each method should have only one responsibility. So let the method return a string which then can be used in the Main().

-
you shouldn't create each time a new Random but reuse an existing.

-
if the range out of which the random numbers should be generated starts at 0 you should use the overloaded Next() method which only takes the exlusive upper bound as a parameter.

-
by using a List the multiple string variables won't be needed and you can take advantage of the string.Join() method

private static Random random = new Random();
private static string GenerateMessage()
{
    IList subMessages = new List(4);   
    subMessages.Add(Greetings[random.Next(Greetings.Length)]);
    subMessages.Add(Compliments[random.Next(Compliments.Length)]);
    subMessages.Add(Garments[random.Next(Garments.Length)]);

    int chance = random.Next(1, 11);
    if (chance < 3)
    {
        subMessages.Add(Farewells[new Random().Next(Farewells.Length)]);
    }

    return string.Join(" ", subMessages);
}

Edit based on the comment


Why do you have a 4 in the parentheses in this line: IList subMessages = new List(4); I removed the 4 to see if the code would still work and it did...

The 4 is pretty bad code style if another programmer needs to ask. Thats what a magic number is which I should have extracted to a meaningful constant like so

private static readonly int maxMessages = 4;

and then used it like so

IList subMessages = new List(maxMessages);

now you know what it is, but you are still missing what is for. That is shown if you take a look at the documentation for this overloaded constructor of the List you will see that this is the initial capacity of the List which is explained more in the Remarks section.


The capacity of a List is the number of elements that the List can hold. As elements are added to a List, the capacity is automatically increased as required by reallocating the internal array.

And the reason why I used it is shown here


If the size of the collection can be estimated, specifying the initial capacity eliminates the need to perform a number of resizing operations while adding elements to the List.

StackExchange Code Review Q#112685, answer score: 6