trjhtr

I'm fairly new to programming and I decided to make a small C# console program for practice. The program asks for user input and gives a random "compliment", it also has a small chance to give a bonus "compliment". How can I improve my code and also what whould you recomend me to try adding? -ps. the "compliments" are mostly a joke.

My code:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Compliment_giver

class Program

 static void Main(string args)
 
 Greeting();
 start:
 string answer = Console.ReadLine();
 if(answer != "yes" && answer != "no")
 
 Console.WriteLine("Unexpected input, please type either "yes" or "no"");
 goto start;
 
 while(answer == "yes")
 
 Console.Clear();
 string result = RandomCompliment();
 Console.WriteLine("n0n",result);
 if (BonusCompliment())
 Console.WriteLine("BONUS COMPLIMENT!nYou are Very Luckyn");
 Console.Write("Would you like another compliment? : ");
 goto start;
 
 

 public static string RandomCompliment()
 
 string compliment = new string[10];
 compliment[0] = "Your hair smells nice";
 compliment[1] = "Your face is ok I guess";
 compliment[2] = "You're probably smarter than a penguin";
 compliment[3] = "Your body appears to be human";
 compliment[4] = "You seem pleasant";
 compliment[5] = "You likely aren't the most annoying person in the world";
 compliment[6] = "You are talanted... at something possibly";
 compliment[7] = "Your eyes are of good color";
 compliment[8] = "Your are the best looking person in a room by yourself";
 compliment[9] = "You are right all the time, exept only when you are not";

 Random number = new Random();
 int rng = number.Next(0, 10);

 string _result = compliment[rng];

 return _result;
 

 public static void Greeting()
 
 Console.WriteLine("Welcome to the Compliment Giver 3000!");
 Console.WriteLine("This program is designed to give free compliments to make you feel better");
 Console.Write("Do you want a compliment? yes/no: ");
 

 public static bool BonusCompliment()
 
 Random percentage = new Random();
 int bonus = percentage.Next(1, 101);

 if (bonus <= 3)
 return true;
 else
 return false;
 

Don't be discouraged by how much I have written. I'm a professional software developer and my colleagues make fun of me because of how pedantic my code reviews are. I think what you've done is actually pretty neat for a first program, and I like that you've obviously made a conscious effort to break things into useful functions (a lot of beginners don't do this well).

I always think people shouldn't pull punches early on in code reviews because it saves agro later on. Some of my points (particularly the formatting/style/naming points) will feel like pedantry, but the issue is that tiny projects become small projects become moderately sized projects and before you know it you have got a beast on your hands. So its worth getting the little stuff right early.

Formatting

• Typically the class should be indented inside the namespace. having two braces at the same indentation level as you have done at the end of the snippet looks strange.


• the second line of bool BonusCompliment() is not indented correctly

Naming

Choose a naming convention and stick to it religiously. This will make your code easier to read.

• 
  compliment is a sequence of compliments so should be plural


• Don't abbreviate names unless its really obvious (int rng) as it makes your code harder to read. I had to guess that this stands for "random number generator", but even unabbreviated its not named correctly - it is the value returned by the generator, not the generator itself. Personally I'd probably have called it index
  


• Why the underscore in string _result? its a common convention in some codebases to prefix class variables with an underscore as a way of distinguishing them, but this is a local variable.


• C# code doesn't typically use underscores to separate words in identifier names, so your namespace looked a little out of place to me

Style

Style points are usually controversial so others might not agree with the observations I've made. Look into the point, research the opinions that are out there and form your own opinion. Once you have done this make sure you are always consistent.

• You don't brace single-line if statements. Many programmers (including me) consider this to be poor practice - have a look at this




• Number literals within code blocks are known as magic numbers which is a practise which is generally discouraged because they make the code harder to read. For example with (bonus <= 3) it is not really clear to the casual reader what the significance of the number '3' is. Is it the age of mum's recently purchased second hand car? The temperature that day? Give a magic number meaning by defining it as a class constant outside the method. This also means that if this number changes at a later date, the change only needs to be made in one place, reducing the possibility of errors.

public class Program 
 private const int MumsBirthday = 3;

 public bool IsItMumsBirthday(int dayOfMonth) 
 return dayOfMonth == MumsBirthday;
 

Simplifications

• 
  public static string RandomCompliment() can just return compliment[rng]; rather than saving it into a variable and returning that


• 
  BonusCompliment() doesn't need the if/else. Just use return bonus <= 3
  

Misc

• Using labels and the goto keyword is generally frowned upon in most programming languages that implement it - you can google for a fair bit of discussion on this point. As with all these kind of 'rules of thumb', if you think you have found a genuine use case that can't be better represented with other language constructs, by all means use goto. Typically though, the behaviour you are trying to implement is done with do/while like this

bool wantsMoreCompliments = true;
while(wantsMoreCompliments) 
 string answer = null;
 do 
 if(answer != null) 
 // Write error message to console
 
 answer = Console.ReadLine()
 while(answer != "yes" && answer != "no")
 // Do something with answer
 // Decide if wantsMoreCompliments

• Its also worth noting that the use of goto here effectively turns your while loop into an if block - there is never going to be an instance where it loops more than once, since it gets to the end and simply jumps out of the loop.


• in public static string RandomCompliment() you create an array of compliment strings. In most situations you will handle as a beginner, sequences of objects should be represented by a List<T> data structure rather than an array. For some more detailed discussion on this topic see here.


• Consider using a collection initialiser for the compliments list as this is much less verbose than the way you have done it:

List<string> compliments = new List<string> 
 "Your hair smells nice",
 "Your face is ok I guess"
 // More compliments
;

Edit

In answer to your question about how to use the list with the random number generator, you would do it like this:

assuming compliments is the list we defined above:

int index = number.Next(compliments.Count);
return compliments[index];

All of Ben Wainwright's pointers are fantastic.

There are, however, two things I believe are important, which were omitted.

1. 
   

   Your use of Random may cause significant headaches in the future. Granted, in your use case, it's extremely unlikely to be an issue, it's a good thing to understand thoroughly, as it's not something you're likely to discover on your own. Here's a detailed explanation of the problem.
   
   In essence, you should not create new Random instances every time you want to generate a random number. You should share instances (but not across threads), to ensure you don't continually generate the same 'random' number. In this case, I'd re-work your code to be:

   
   
   

   class Program
   
    private static readonly Random _random = new Random();
   
    public static string RandomCompliment()
    
    // ...
    int rng = _random.Next(0, 10);
   
    string _result = compliment[rng];
   
    return _result;
    
   
    public static bool BonusCompliment()
    
    int bonus = _random.Next(1, 101);
    if (bonus <= 3)
    return true;
    else
    return false;
    
   
   

   
   
   

   Note that we can mark the field as readonly^* to better clarify our intent that _random should be a single instance.

   
   



2. This is somewhat minor in comparison, but you're creating a new array every time you ask for a random compliment. You don't really need to do all that work each time, you need only do it once. Again, I'd refactor this out of the method, so it's only created once, and have the method read from it.

^* Thanks to Jesse C. Slicer for the suggestion!

In addition to Ben Wainwright's excellent and comprehensive review, I would add:

1. When accepting user input, you should always clean it up and normalise it. Trim white spaces, and convert to either uppercase or lowercase. If the user entered "YES", your program wouldn't match to "yes", for example.




2. When returning the result of a boolean expression it is clearer and less error prone to simply return the expression. For example,

Instead of:

if (bonus <= 3)
 return true;
else
 return false;

simply do:

return bonus <= 3;