9
\$\begingroup\$

So this is a pretty simple code, I think it was?

It asks for the user's birth day and month, and gives it back with the day a discount reminder email will be sent (the day before their birthday)

Now I tried to optimize as much as I could, possible wrong inputs and if the user's birthday is the first of a month

Even though I'm still pretty new to coding, I still want you to criticize my code as much as I could, I would like to improve as much as I can

using System.Text.RegularExpressions;
using System.Linq;

namespace Exercice14
{
    class Program
    {
        static void Main(string[] _)
        {
            // declaring some variables
            int birthDay;
            int reminderDay;
            string suffix = "th";
            string reminderSuffix = "th";
            string birthDayT;
            string birthMonth;
            string reminderMonth;
            string[] months = {"January", "February", "March", "April", "May", "June", "July", "August", "September", 
                                 "October", "November", "December" };
            bool exceptionFirst = false;

            // prompts for birth month and capitalize first letter
            Console.WriteLine("Hello User!");
            Console.Write("Please enter your birth month in letters: ");
            birthMonth = Console.ReadLine();
            birthMonth = char.ToUpper(birthMonth[0]) + birthMonth.Substring(1);


            // check if birth month contains only letters
            while(Regex.IsMatch(birthMonth, @"^[a-zA-Z]+$") == false)
            {
                Console.WriteLine("Birth month should only contain letters!");
                Console.Write("Please enter your birth month in letters: ");
                birthMonth = Console.ReadLine();
                birthMonth = char.ToUpper(birthMonth[0]) + birthMonth.Substring(1);
            }

            // check if month is right
            while (months.Contains(birthMonth) == false)
            {
                Console.WriteLine("Invalid month?! Please enter a valid english month");
                Console.Write("Please enter your birth month: ");
                birthMonth = Console.ReadLine();
                birthMonth = char.ToUpper(birthMonth[0]) + birthMonth.Substring(1);
            }

            // prompts for birth day
            Console.Write("Please enter your birth day in numbers: ");
            birthDayT = Console.ReadLine();

            // check for valid day 
            while (int.TryParse(birthDayT, out int _) == false)
            {
                Console.WriteLine("Invalid argument! Please enter day in numerals");
                Console.Write("Please enter your birth day in numbers: ");
                birthDayT = Console.ReadLine();
            }

            // check for valid day number
            while (int.Parse(birthDayT) < 1 || int.Parse(birthDayT) > 31)
            {
                Console.WriteLine("Invalid date! Please enter a day between 1 and 31");
                Console.Write("Please enter birth day in numbers: ");
                birthDayT = Console.ReadLine();
            }

            // assign birth day to variable once tested
            birthDay = int.Parse(birthDayT);

            // set reminder day and month
            reminderDay = birthDay - 1;
            reminderMonth = birthMonth;

            // check which suffix to use for days AND calculate reminder day and month if exception
            if (birthDay == 1) //exception
            {
                exceptionFirst = true;
                suffix = "st";
                reminderMonth = months[Array.IndexOf(months, birthMonth) - 1];
            }
            if (birthDay == 2)
            {
                suffix = "nd";
                reminderSuffix = "st";
                reminderDay = 1;
            }
            if (birthDay == 3)
            {
                suffix = "th";
                reminderSuffix = "nd";
            }
            if (birthDay > 3)
            {
                suffix = "th";
                reminderSuffix = "th";
            }

            // print values
            Console.WriteLine();
            Console.WriteLine("Yer birthday is on the " + birthDay + suffix + " of " + birthMonth );

            if (exceptionFirst == true)
            {
                Console.WriteLine("A reminder email for your birthday discount " +
                    "\nwill be sent on the last day of " + reminderMonth);
            }
            else
            {
                Console.WriteLine("A reminder email for your birthday discount " +
                    "\nwill be sent on the " + reminderDay + reminderSuffix + " of " + reminderMonth);
            }

        }
    }
}
\$\endgroup\$
0

3 Answers 3

5
\$\begingroup\$

Notes :

  • Months array
  • Regex
  • multiple loops and redundant validations.
  • Direct parsing without validations.

As Roland mentioned, you don't need to redefine what is already existed, nor handling the conversion of dates manually. You need to focus on using what .NET already has, if you don't know, google before you start coding. This way, you will avoid making major changes.

You take two inputs from the user, then you only need 2 validations process. While in your code you're doubling that!, which is unnecessary if you implement it correctly.

Let's start with the month validation, user can input a short name month, or full name, or even a number. As you're dealing with string, you need to take the inputs possibilities, even if you tried to restrict the input, there is still a chance of an invalid input with is the unknown case! So, you will focus on covering known cases, which you already mostly did.

The repeated issue that you are unaware of is that you're assigning and processing then validating the user input, so you need to reverse that. First validate, then process based on that validation.

Here is an example of your process of validation :

// prompts for birth month and capitalize first letter
Console.WriteLine("Hello User!");
Console.Write("Please enter your birth month in letters: ");
birthMonth = Console.ReadLine();
birthMonth = char.ToUpper(birthMonth[0]) + birthMonth.Substring(1);


// check if birth month contains only letters
while(Regex.IsMatch(birthMonth, @"^[a-zA-Z]+$") == false)
{
    Console.WriteLine("Birth month should only contain letters!");
    Console.Write("Please enter your birth month in letters: ");
    birthMonth = Console.ReadLine();
    birthMonth = char.ToUpper(birthMonth[0]) + birthMonth.Substring(1);
}
  1. First you asked a user for an input
  2. then you directly get the first char assuming it's a valid string
  3. then you validate it with regex.
  4. if invalid, you do the same steps 1 to 3 until it's valid letter.

what happens if birthMonth is empty or null? it'll throw IndexOutOfRangeException because of birthMonth[0] and if the birthMonth is NULL then it'll also throw a null exception.! these are basic validations which need to be validated before processing.

you've applied the same process to the reset. you need to validate the string first using string.IsNullOrEmpty or string.IsNullOrWhiteSpace or if you prefer to do it manually you can do this

if(birthMonth != null && birthMonth.Length > 0)

for the month part, you don't need the array, you need to use DateTime instead. You can use something like this :

// handle the month conversion
// acceptable inputs : short name, full name, month number 
private static bool TryGetMonth(string month, out DateTime date)
{
    date = new DateTime();

    if(string.IsNullOrEmpty(month))
    {
        return false;       
    }

    // default datetime format 
    var format = "dd MMMM yyyy HH:mm:ss tt";

    // if user enters a repersental month number then adjust the format
    if(int.TryParse(month, out int monthInt))
    {
        format = "dd M yyyy HH:mm:ss tt";
    } 
    else if(month.Length <= 3 && !month.Equals("May", StringComparison.OrdinalIgnoreCase))
    {
        format = "dd MMM yyyy HH:mm:ss tt";
    }

    return DateTime.TryParseExact($"01 {month} 2020 00:00:00 AM", format, CultureInfo.InvariantCulture, DateTimeStyles.None, out date);
}

the DateTime.TryParseExact will handle the conversion, and would return a valid date if the input meets the parsing requirements. Then, from the dateTime, you can have access to its values like month name, number ..etc.

Also, when parsing integers, use int.TryParse to check the validity of the integer first, then extract the parsed integer. This would avoid throwing undesired exceptions.

here is an untested revision of your code using the TryGetMonth method above along with using DateTime to demonstrate my points:

// prompts for birth month and capitalize first letter
Console.WriteLine("Hello User!");
Console.Write("Please enter your birth month in letters: ");

while(TryGetMonth(Console.ReadLine(), out DateTime monthDate ) == false)
{
    Console.WriteLine("invalid month");
    Console.WriteLine("Please enter your birth month name (short or full name) or number");
}

// prompts for birth day
Console.Write("Please enter your birth day in numbers: ");

while(int.TryParse(Console.ReadLine(), out int birthDay) && (birthDay > 0  && birthDay <= 31))
{
    Console.WriteLine("Invalid argument! Please enter day between 1-31 in numerals");
    Console.WriteLine("Please enter your birth day in numbers: ");
}


DateTime birthDate = new DateTime(DateTime.Now.Year, monthDate.Month, birthDay);
DateTime reminderDate = birthDate.AddDays(-1);

string suffix;
string reminderSuffix;
string msg; 

switch(birthDate.Day)
{
    case 1:
        suffix = "st";
        reminderDate = reminderDate.AddMonths(-1);
        break;
    case 2:
        suffix = "nd";
        reminderSuffix = "st";
        break;
    case 3:
        suffix = "rd";
        reminderSuffix = "nd";
        break;
    case 4:
        suffix = "th";
        reminderSuffix = "rd";
        break;
    default: 
        suffix = "th";
        reminderSuffix = "th";
}

if(birthDate.Day == 1) {
    msg = $"A reminder email for your birthday discount \nwill be sent on the last day of {reminderDate.ToString("MMMM")}";
} else {
    msg = $"A reminder email for your birthday discount \nwill be sent on the {reminderDate.Day}{reminderSuffix} of {reminderDate.ToString("MMMM")}";
}

// print values
Console.WriteLine();
Console.WriteLine($"Your birthday is on the {birthDate.Day}{suffix} of {birthDate.ToString("MMMM")}" );
Console.WriteLine(msg);

// reminderDate.ToString("MMMM") would return month name
// "MMMM" for full name and "MMM" for short name (e.g. June and Jun)
\$\endgroup\$
0
2
\$\begingroup\$

The code you wrote is quite large for a beginner, and its overall structure is easy to read and understand. That's quite good.

Of course there are lots of things you can improve. One thing that comes to mind is to split the code into several separate methods. A good candidate is the part where you convert a day and month into a string. Right now almost half of the code is concerned with keeping track of st, nd, rd, th. That is distracting. You could define a method like this:

namespace Exercice14
{
    class Program
    {
        /// The month goes from 1 to 12, the dayOfMonth goes from 1 to 31.
        /// There is no check for invalid numbers.
        string DateToString(int month, int dayOfMonth)
        {
            string ordinal = "th";
            if (dayOfMonth == 1)
                ordinal = "st";
            else if (dayOfMonth == 2)
                ordinal = "nd";

            return "the " + dayOfMonth + ordinal + " of " + MonthNames[month - 1];
        }
    }
}

This way, you don't need to handle these ordinal suffixes in the rest of the code. This means you cannot accidentally mix up the ordinal of the birthday and the ordinal of the reminder day, and this already makes your code more reliable.

Another good thing is that you can write automatic tests for this little method.

namespace Exercice14
{
    class Program
    {
        void TestDateToString(string expected, int month, int dayOfMonth)
        {
            string actual = DateToString(month, dayOfMonth);
            if (actual != expected)
            {
                throw new InvalidOperationException(
                    "Expected '" + expected + "'"
                    + " for month " + month + " and day " + dayOfMonth + ","
                    + " got '" + actual + "'.");
            }
        }

        void TestDateToString()
        {
            TestDateToString("1st of January", 1, 1);
            TestDateToString("22nd of July", 7, 22);
            TestDateToString("31st of December", 12, 31);
        }
    }
}

Having all these ingredients, you can now simply add TestDateToString(); at the beginning of the Main method, just before the // declaring comment. This way, your program will only run if the tests run successfully.

This is only the first step to writing reliable code. The next step is to split your code into the main code and the testing code, and use a unit test framework such as NUnit to run your tests.

Whenever you are doing date calculations in your own code, you are doing something wrong. Date calculations are incredibly tricky, regarding time zones, daylight saving time, leap years, leap seconds, and so on. Therefore you should make use of the .NET standard library, which already defines a type called DateTime. You need to do 3 things:

  1. construct a DateTime from its parts (month, day of month)
  2. calculate the day before that date (using DateTime.AddDays(-1))
  3. format this date back into a string (possibly using this code)

To cut the long story short: Your code works for a few cases but crashes for some others or even prints wrong results. Writing the automatic tests and thinking of possible interesting test cases will help you find and fix these bugs.

\$\endgroup\$
2
\$\begingroup\$

Your Main method is too long, to be able to even see what it does I had to read the whole method.

You did explain what it does:

It asks for the user's birth day and month, and gives it back with the day a discount reminder email will be sent (the day before their birthday)

However by using classes and sub-method your Main-method could be telling you exactly this.

class Program
{
    class MyDate {
       // ...
    }

    static void Main(string[] _)
    {
        MyDate birthday = ask_for_birthday();
        MyDate email_day = day_before(birthday);
        output_email_message(email_day);
    }
}

If you structure your code like that it will be much easier to critique and analyse further. If you use the already existing Date class of C# the day_before() method will probably pretty simple to implement even.

\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.