Random Message Generator using Arrays

Question

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

This code will piece together a random message with the contents in 4 separate arrays: Greetings, Compliments, Garments, Farewells.

There is a 20% chance of the randomly generated message including a farewell at the end of the message. There is an 80% chance for the message not to include a farewell exclamation in the message.

How can I clean this code up and make it produce the same product, but more efficiently? Any help is greatly appreciated! :)

Answer 1

• 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<string> 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<string> subMessages = new List<string>(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<string> subMessages = new List<string>(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<string> subMessages = new List<string>(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<T> you will see that this is the initial capacity of the List<T> 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.

• Owen: Thanks a lot for the detailed answer to my question. I really appreciate!

Answer 2

In addition to Heslacher's advice, I'd also suggest you to move most of the logic to a separate class, e.g. MessageGenerator, and call that one from the Main().

Yes, the current program is small, and even smaller when you apply Heslacher's advice, but IMO you should get in the habit of keeping (削除) the Main() of (削除ここまで) your Console's Program as small as possible, and more importantly have dedicated classes that handle your logic.

Note that currently the data in Greetings etc. is fairly limited, but what if there are dozens of different message parts in the future? Consider moving those arrays to a separate class, and perhaps even to a config file.

---

Much of your message is also repeated in the various parts. Each of your Greetings contains ", you have a", each of your Garments contains a ".". Those should be moved to the line that generates the message, perhaps something like this:

return string.Format("{0}, you have a {1} {2}.{3}", 
 greeting, compliment, garment, farewell);

Answer 3

If anyone wants to use this in a C# form, here is the code for that and it also works with a text so you'll have to create one.

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
namespace Lavender_Voice_Tester
{
 public partial class Form1 : Form
 {
 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;
 public Form1()
 {
 InitializeComponent();
 }
 private void Form1_Load(object sender, EventArgs e)
 {
 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;
 textBox1.Text = (Message); 
 }
 //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;
 textBox1.Text = (Message);
 }
 }
 }
}

• AlexV: Welcome to Code Review! Code Review is a place to get feedback from others regarding your code. Since you do not review the question, this is not considered as a valid answer (see How to Answer). Also have a look at Why are alternative solutions not welcome? .