11
\$\begingroup\$

I made this small system that checks if you're old enough to drive. I'm new to C#, but I do know most of the basics since I also know Java. I'm trying to make my code more efficient. Any suggestions or help would be appreciated.

using System;
using System.Threading;

class EfficientCode
{

  static int age = 0;

    static void Main(string[] args)
    {
        Console.WriteLine("This is just a simple project that takes simple and small information from you and uses it to process the information and checks whether you're allowed to drive or not. It also tells you how long is left till you drive.");

        Thread.Sleep(8000); // adds a 8 second delay so that the user could first read the introduction

        Start(); // after the 8 second delay it starts the system
    }

    static void Start()
    {
        Console.WriteLine("Enter your first name");
        String firstName = Console.ReadLine();

        Console.WriteLine("Enter your middle name");
        String middleName = Console.ReadLine();

        Console.WriteLine("Enter your last name");
        String lastName = Console.ReadLine();

        Console.WriteLine("Enter your age");

        try
        {
            age = Convert.ToInt32(Console.ReadLine());
        }
        catch
        {
            Console.WriteLine("ERROR: Input AGE must be a number!");
            Console.WriteLine("Reinitializing questions.");

            Thread.Sleep(3000); // this adds a delay of 3 seconds

            Start(); // restarts the questions
        }

        if (ConfirmInformation(firstName, middleName, lastName, age))
        {
            if (age >= 18)
            {
                Console.WriteLine("Congratulations! You're already old enough to drive.");

                Environment.Exit(0); // Ends the system
            }
            else if (age == 17)
            {
                Console.WriteLine("You're almost there! You can get a learners permit.\nYou have 1 more year left to get an official drivers license!");

                Environment.Exit(0); // Ends the system
            }
            else
            {
                Console.WriteLine("You'll get there. You're not old enough to have a license.");

                int yearsLeft = 18 - age; // gets the years left to drive

                Console.WriteLine("You still have " + yearsLeft + " years left to get a license.");

                Environment.Exit(0); // Ends the system
            }
        }
        else
        {
            Console.WriteLine("Reinitializing questions.");

            Thread.Sleep(3000); // this adds a delay of 3 seconds

            Start(); // restarts the questions
        }
    }

    static bool ConfirmInformation(String firstName, String middleName, String lastName, int age)
    {
        Console.WriteLine("======================================");
        Console.WriteLine("INFORMATION CHECK:");
        Console.WriteLine("");
        Console.WriteLine("FIRST NAME: " + firstName);
        Console.WriteLine("MIDDLE NAME: " + middleName);
        Console.WriteLine("LAST NAME: " + lastName);
        Console.WriteLine("AGE: " + age);
        Console.WriteLine("");
        Console.WriteLine("Are all those information correct? (Y/N)");
        Console.WriteLine("======================================");

        String booleanInput = Console.ReadLine();

        if (booleanInput.Equals("yes", StringComparison.OrdinalIgnoreCase) || booleanInput.Equals("y", StringComparison.OrdinalIgnoreCase))
        {
            return true;
        }
        else if (booleanInput.Equals("no", StringComparison.OrdinalIgnoreCase) || booleanInput.Equals("n", StringComparison.OrdinalIgnoreCase))
        {
            return false;
        }
        else
        {
            Console.WriteLine("ERROR: Input must be either YES or NO.");

            return false;
        }

        return false;
    }

}
\$\endgroup\$
  • 9
    \$\begingroup\$ Some comments regarding usability, not technical code quality: Inserting artificial delays, having the user re-enter all data when just one answer can be parsed, requesting a specific first-middle-last name format all contribute to a bad user experience. If the user can correct invalid data after the confirmation step, they should be able to keep already entered correct information. \$\endgroup\$ – Hans-Martin Mosner Oct 19 at 10:17
  • 4
    \$\begingroup\$ Falsehoods programmers believe about names with examples \$\endgroup\$ – CJ Dennis Oct 20 at 5:06
17
\$\begingroup\$
  • class EfficientCode could be improved by naming the class by the purpose of the code. Otherwise your naming is good.
  • static int age = 0; should be a local variable in Start()
  • Thread.Sleep(8000); // adds a 8 second delay so that the user could first read the introduction shouldn't be needed because you aren't clearing the Console hence the user could still read it.
  • Instead of try..catch you could take advantage of the int.TryParse() method which returns a bool indicating the success of "converting" the string to an int. If you would use int.TryParse like below you could omit the restarting in case of an error. If the call succeeds age holds the integer.

    while(!int.TryParse(Console.ReadLine(), out age))
    {
        Console.WriteLine("ERROR: The entered AGE must be a number!");
        Console.WriteLine("Enter your age");
    }
    
  • ConfirmInformation() is doing a little bit to much. You should split it into 2 methods. One composing the output and one printing the output and asking for confirmation.

    private static string ComposeOutput(String firstName, String middleName, String lastName, int age)
    {
        StringBuilder sb = new StringBuilder(100);
        sb.AppendLine("======================================")
          .AppendLine("INFORMATION CHECK:")
          .AppendLine()
          .AppendLine($"FIRST NAME: {firstName}")
          .AppendLine($"MIDDLE NAME: {middleName}")
          .AppendLine($"LAST NAME: {lastName}")
          .AppendLine($"AGE: {age}")
          .AppendLine()
          .AppendLine("Are all those information correct? (Y/N)")
          .AppendLine("======================================");
    
        return sb.ToString();
    }  
    

    This method uses the StringBuilder class. The AppendLine() method is returning the StringBuilder itself hence we can use the method calls fluently. It uses $-string interpolation as well.

    The former ConfirmInformation() method could then look like so

    private static bool ConfirmInformation(string information)
    {
        Console.Write(output);
    
        while (true)
        {
            string input = Console.ReadLine().ToLowerInvariant();
            if (input == "y" || input == "yes")
            {
                return true;
            }
            if (input == "n" || input == "no")
            {
                return false;
            }
    
            Console.WriteLine("ERROR: Input must be either YES or NO.");
            Console.WriteLine();
            Console.Write(output);
        }
    }
    

    I have made a while(true) loop here because your user wouldn't want to restart the whole process if they made a mistake.
    As a sidenode, the variable String booleanInput = Console.ReadLine(); is misleading. If Sam the maintainer would read this method the purpose of that variable wouldn't be seen at first glance.

\$\endgroup\$
  • 1
    \$\begingroup\$ @OP Int.tryParse() is the way to go. Just want to add that when you do catch exceptions, you should always catch the specific Exceptions you're looking for. OP's current code will cause a confused user & endless loop if something else goes wrong with ReadLine. \$\endgroup\$ – dustytrash Oct 18 at 12:52
  • \$\begingroup\$ Thank you so much! This helped me alot. I have a question, when you did CSharp .AppendLine($"FIRST NAME: {firstName}") .AppendLine($"MIDDLE NAME: {middleName}") .AppendLine($"LAST NAME: {lastName}") .AppendLine($"AGE: {age}") what does the $ do before the string? Thanks! \$\endgroup\$ – Zxoir Oct 18 at 14:52
  • 1
    \$\begingroup\$ Read below the code. I have added a link for explaination. \$\endgroup\$ – Heslacher Oct 18 at 14:54
  • \$\begingroup\$ Ah yes I see it now, thanks again! \$\endgroup\$ – Zxoir Oct 18 at 14:55
  • 2
    \$\begingroup\$ I upvoted this but am surprised at some of your naming. It's odd for me to see a signature of ConfirmInformation(string output) where the input parameter is named output! I'd suggest renaming such "Output" something like "UserPrompt" or "UserPromptMessage". That would be method ComposeUserPrompt and ConfirmInformation(string userPrompt). \$\endgroup\$ – Rick Davin Oct 19 at 14:59
12
\$\begingroup\$

First: you shouldn't use recursion to loop like that. It often ends up confusing and can technically lead to errors like stack overflows if it goes on forever. If you need to loop, use a for loop or a while loop.

Most of the code looks like it can be in main, I don't see a reason why start has to exist. Confirmation being separate is fine.

You don't need to write out a try catch block for your age parsing. All the basic types in .NET have handy TryParse methods.

if (!int.TryParse(Console.ReadLine(), out int age))
{
    /// ...
}

You put a useless return false at the end of your confirmation code. As for the rest, there's a lot of ways we can handle a bunch of cases for your confirmation inputs cleanly. I think a switch is good in this case. A dictionary would also work.

You could also just make the string lowercase then just check to see if the first character is y or n in your case, but we're just applying general practices.

string input = Console.ReadLine();

switch (input.ToLower()) // lowercase so it works
{
    case "yes":
    case "y":
        return true;

    case "no":
    case "n":
        return false;

    default:
        // bad input code
        return false;
}

You repeated Environment.Exit 3 times. Making the legal driving age a constant is a good idea. Just think, we could change it work for other countries!

If the age isn't hardcoded, instead of checking for 17 or 18 you just check to see how many years they have left until they can drive.

const int LEGAL_DRIVING_AGE = 18; // declared somewhere, like the main class

int yearsLeft = LEGAL_DRIVING_AGE - age;

if (yearsLeft <= 0)
    Console.WriteLine("You can drive!");

else if (yearsLeft == 1)
    Console.WriteLine("You can get a learners permit...");

else
    Console.WriteLine($"You're not old enough yet. You still have {yearsLeft} years to go!");

Environment.Exit(0);

Hope that helps.

\$\endgroup\$
  • \$\begingroup\$ Thanks! I'll be using the switch way more now :) \$\endgroup\$ – Zxoir Oct 18 at 14:54
  • 1
    \$\begingroup\$ A real-world example of how this could lead to a stack overflow: A broom fell onto a keyboard, it was positioned so it held down the enter key on the number pad. (This was a networked program and caused considerable confusion in the factory for a few hours until the problem was identified as it was swallowing every task at the assembly station.) \$\endgroup\$ – Loren Pechtel Oct 20 at 6:06
5
\$\begingroup\$

Here is an overdone refactoring that I did. It just shows how the code could be modularized, but it is a bit too heavy-duty for a small example.

using System;

namespace OldEnoughToDrive
{
    /// <summary>
    /// A container to hold attributes of a person
    /// </summary>
    internal class Person
    {
        public const int LegalAge = 18;
        public string FirstName;
        public string MiddleName;
        public string LastName;
        public int Age;

        public override string ToString()
        {
            return $"FIRST NAME: {FirstName}\n"
                + $"MIDDLE NAME: {MiddleName}\n"
                + $"LAST NAME: {LastName}\n"
                + $"AGE: {Age}";
        }
        public int GetYearsLeftToDrive()
        {
            return LegalAge - Age;
        }
    }

    internal class UserInput
    {
        /// <summary>
        /// Gets a string from the user
        /// </summary>
        /// <returns>The user's input</returns>
        public static string String(string prompt)
        {
            Console.WriteLine(prompt);
            return Console.ReadLine();
        }

        /// <summary>
        /// Gets a natural number from the user
        /// </summary>
        /// <param name="errorMsg">Error message to show if the prompt is not a natural number</param>
        /// <returns>A natural number from the user's input</returns>
        public static int NaturalNumber(string prompt, string errorMsg)
        {
            Console.WriteLine(prompt);
            var userResponse = Console.ReadLine();
            int actualInt;
            while (!int.TryParse(userResponse, out actualInt) || actualInt <= 0)
            {
                Console.WriteLine(errorMsg);
                userResponse = Console.ReadLine();
            }

            return actualInt;
        }

        /// <summary>
        /// Gets a boolean from the user
        /// </summary>
        /// <param name="errorMsg">Error message to show if the input is not valid</param>
        /// <returns>The boolean that the user entered</returns>
        public static bool Boolean(string prompt, string errorMsg)
        {
            Console.WriteLine(prompt);
            while (true)
            {
                string input = Console.ReadLine()?.ToLower();
                switch (input)
                {
                    case "y":
                    case "yes":
                        return true;
                    case "n":
                    case "no":
                        return false;
                    default:
                        Console.WriteLine(errorMsg);
                        break;
                }
            }
        }
    }

    internal class Program
    {
        private static void Main()
        {
            Console.WriteLine("This is just a simple project that takes simple and small information " +
                "from you and uses it to process the information and checks whether you're allowed to " +
                "drive or not. It also tells you how long is left till you drive.");

            while (true)
            {
                var person = GetPersonInput();

                if (ConfirmInformation(person))
                {
                    PrintLicenseStatus(person);
                }
                else
                {
                    Console.WriteLine("Reinitializing questions.");
                }
            }
        }

        /// <summary>
        /// Prints the status of the license for this person (what type of license they can hope
        /// for, or if they do not have one)
        /// </summary>
        /// <param name="person">The person to check</param>
        private static void PrintLicenseStatus(Person person)
        {
            if (person.Age >= 18)
            {
                Console.WriteLine("Congratulations! You're already old enough to drive.");
            }
            else if (person.Age == 17)
            {
                Console.WriteLine("You're almost there! You can get a learners permit.");
                Console.WriteLine("You have 1 more year left to get an official drivers license!");
            }
            else
            {
                Console.WriteLine("You'll get there. You're not old enough to have a license.");
                Console.WriteLine($"You still have {person.GetYearsLeftToDrive()} years left to " +
                                  "get a license.");
            }
        }

        /// <summary>
        /// Builds a Person class via prompting the user for their personal information
        /// </summary>
        /// <returns>A filled-in valid person object</returns>
        private static Person GetPersonInput()
        {
            var firstName = UserInput.String("Enter your first name");
            var middleName = UserInput.String("Enter your middle name");
            var lastName = UserInput.String("Enter your last name");
            var age = UserInput.NaturalNumber("Enter your age", "ERROR: Input AGE is invalid!");
            return new Person
            {
                FirstName = firstName,
                MiddleName = middleName,
                LastName = lastName,
                Age = age
            };
        }

        /// <summary>
        /// Confirms the user's information by printing it to the screen
        /// </summary>
        /// <param name="person">The person's information</param>
        /// <returns>Whether the user has confirmed their information is valid</returns>
        private static bool ConfirmInformation(Person person)
        {
            Console.WriteLine(
                new string('=', 38)
                + "\nINFORMATION CHECK:\n"
                + person
                + "\n"
                + new string('=', 38)
            );

            return UserInput.Boolean("Are all those information correct? (Y/N)",
                                       "ERROR: Input must be either YES or NO.");
        }
    }
}

What I did was to refactor the app into a few components. The app is focused around getting a Person's information from some UserInput, and those UserInput's were validated and then the user's information was Confirmed. I took those concepts and extracted them into classes and methods.

The validation methods were generalized to be used throughout the app if they need to be used again.

\$\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.