32
\$\begingroup\$

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("\n{0}\n",result);
            if (BonusCompliment())
                Console.WriteLine("BONUS COMPLIMENT!\nYou are Very Lucky\n");
            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;
    }

}
}
\$\endgroup\$
4
  • 7
    \$\begingroup\$ Best c# beginner program I've seen yet. Keep studying! \$\endgroup\$
    – user34073
    Feb 26, 2018 at 3:03
  • 3
    \$\begingroup\$ Spelling / grammar in the compliments (they're hard-coded, so it's technically code, right?): talanted -> talented, Your are -> You are or You're. \$\endgroup\$
    – user673679
    Feb 26, 2018 at 17:09
  • 2
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Mast
    Feb 26, 2018 at 20:14
  • 3
    \$\begingroup\$ @Hosch250 Not to pop your bubble or anything, but it's not often I encounter C# code using arcane constructs like goto in multiple places. More studying is indeed a good idea. \$\endgroup\$
    – Mast
    Feb 26, 2018 at 20:16

3 Answers 3

31
\$\begingroup\$

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];
\$\endgroup\$
6
  • \$\begingroup\$ I apreciate the feedback, originally I tried to put the compliments into a List (and even in a class) but I couldn't find a way to have them return an integer so I can make the result random (or another way to do the same). \$\endgroup\$
    – user161670
    Feb 25, 2018 at 21:38
  • \$\begingroup\$ Hey @Stav_K - I've just edited my answer to include an explanation of how that would be achieved (at the bottom). Hope that helps! \$\endgroup\$ Feb 25, 2018 at 21:47
  • 1
    \$\begingroup\$ ""I always think people shouldn't pull punches early on in code reviews because it saves agro later on."". 100% this. +1 \$\endgroup\$
    – Aster
    Feb 26, 2018 at 9:47
  • \$\begingroup\$ I would like to add that the random number generator (instance of Random) should not be called number, which it still is after your suggestions. In the part Random number = new Random(); int rng = number.Next(0, 10);, besides the abbreviation which should be avoided, the names should logically be the other way round. \$\endgroup\$ Feb 26, 2018 at 10:47
  • 3
    \$\begingroup\$ On your point "One of the useful side effects of following this advice is that you will then be able to declare the list with a collection initialiser which is much less verbose" - This isn't a benefit of using a List<T> over an array. An array also allows this. var compliments = new[] { "a", "b" }; \$\endgroup\$
    – Rob
    Feb 26, 2018 at 11:28
6
\$\begingroup\$

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;
\$\endgroup\$
1
  • 1
    \$\begingroup\$ (Welcome to CR!) (You seem to have missed Ben Wainwright's second Simplification.) \$\endgroup\$
    – greybeard
    Feb 25, 2018 at 23:33
6
\$\begingroup\$

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!

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.