Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am a beginner and I have made a tiny calculator in C# that calculates your age when you specify your birthdate. I'm pretty sure it will look horrible to any developer out there, but hey, that's why I've posted it.

I'd like a general review of this. I'm especially concerned about the message variable and the lines after the try-catch statement.

namespace Age
{
    class Program
    {
        static void Main(string[] args)
        {
            while (true)
            {
                try
                {
                    Console.Write("Enter your birtdate: ");
                    DateTime birthDate = DateTime.Parse(Console.ReadLine());

                    int Days = (DateTime.Now.Year * 365 + DateTime.Now.DayOfYear) - (birthDate.Year * 365 + birthDate.DayOfYear);
                    int Years = Days / 365;
                    string message = (Days >= 365) ? "Your age: " + Years + " years" : "Your age: " + Days + " days";

                    Console.WriteLine(message);
                }
                catch
                {
                    Console.WriteLine("You have entered an invalid date.\n");
                }

                Console.WriteLine("Exit? (y/n)");
                string userValue = Console.ReadLine();

                if (userValue == "y")
                {
                    Environment.Exit(0);
                }
            }
        }
    }
}
share|improve this question
    
Is there something specific that you would like a feedback about your code or you just want a general review ? –  Marc-Andre Mar 17 at 19:52
    
Should this be tagged homework? Leap-years contain 366 days. –  ChrisW Mar 17 at 19:56
    
I just wanted a general review. Still, I'm a little uncertain about the "string message"-variable and the lines after the try-catch statement. I've never really terminated an application like this before. This is not homework, just me programming on hobby basis. –  Commodent Mar 17 at 20:05
add comment

4 Answers 4

Console.Write("Enter your birtdate: ");
DateTime birthDate = DateTime.Parse(Console.ReadLine());

Your catch block looks like it's assuming a ParseException that would be thrown by the above DateTime.Parse call. Thus, it would be better to be explicit about it:

catch(ParseException)
{
    Console.WriteLine("You have entered an invalid date.\n");
}

This way, if another exception type is thrown somewhere else in the loop, you will not be displaying a misleading message.

Now, how about using DateTime.ParseExact, and specify what would be the expected date format?

Console.Write("Enter your birtdate (yyyy-MM-dd): ");
DateTime birthDate = DateTime.ParseExact(Console.ReadLine(), "yyyy-MM-dd", CultureInfo.InvariantCulture);

That's all great, too many things are happening in a single line of code here:

  • Controle.ReadLine() is getting the console input
  • DateTime.Parse | DateTime.ParseExact is parsing the console input

You should break this down into 2 separate instructions:

Console.Write("Enter your birtdate (yyyy-MM-dd): ");
var input = Console.ReadLine();
var birthDate = DateTime.ParseExact(input, "yyyy-MM-dd", CultureInfo.InvariantCulture);

Note that I'm using var for brevety here, input is a string and birthDate is a DateTime.

The next step would be to extract a method out of these few lines - have a method called GetBirthDate that returns a DateTime, or throws a ParseException that your catch block can handle.

share|improve this answer
add comment
  1. You should give the user some format to follow when entering their birthday

    Console.Write("Enter your birthdate (MM/DD/YYYY): ");
    
  2. You should give the user a way to close the program. While this isn't incredibly necessary for simple learning programs, it is a good habit to get into.

  3. You should avoid at all costs the throwing of exceptions, they are slow and break up your programs flow. Certainly use them where necessary, but this is not one of those places. In the following snippet, I've declared the DateTime object, and a Boolean which will tell us if the string entered by the user was able to be parsed. Notice how I am calling the TryParse Method. The TryParse method takes in a string, and an out DateTime object. This is important, the out keyword is a bit more advanced than you have come accross. It is a keyword which implements functionality programmers were used to in C using pointers. Suffice to say that this function will set whatever object you pass in, to be the parsed value.

    DateTime birthDate;
    bool succeeded = DateTime.TryParse(Console.ReadLine(), out birthDate);
    

    Because the TryParse method returns a bool you can just use this in an if statement which is conditional on the TryParse returning true.

    if(DateTime.TryParse(Console.ReadLine(), out birthDate))
    {
        // more stuff
    }
    else
        Console.WriteLine("You have entered an invalid date.");
    
  4. I noticed you used \n in your program. While I will say that I am guilty of using this all the time, you should actually use Environment.NewLine. The reason is because of in-consistency of newline/carriage returning in different programs. Environment.NewLine always uses the proper line ending.

    Console.WriteLine("You have entered an invalid date." + Environment.NewLine);
    
  5. You can use + or - operators on DateTime objects which results in a TimeSpan object

    TimeSpan age = DateTime.Now - birthDate;
    

    TimeSpan objects have properties like Days, Hours. the calculation done, does take into consideration leap years.

  6. Printing to the console can be made easier by sending parameters to be inserted into your string.

    Console.WriteLine("Your age: {0} years and {1} days", (int)(age.Days/365.25), age.Days % 365.25);
    

Over all, your program could look like this. Note: I didn't programatically address the issue of the user escaping your program, I noticed you updated your OP with that.

while (true)
{
    Console.Write("Enter your birtdate (MM/DD/YYYY): ");
    DateTime birthDate;
    if (DateTime.TryParse(Console.ReadLine(), out birthDate))
    {
        TimeSpan age = DateTime.Now - birthDate;
        Console.WriteLine("Your age: {0} years and {1} days", (int)(age.Days/365.25), age.Days % 365.25);
    }
    else
        Console.WriteLine("You have entered an invalid date." + Environment.NewLine);
}

Edit: This is of-course personal taste, but I dislike (what I consider to be unnessary brackets taking up newlines).

else
{
    Console.WriteLine("You have entered an invalid date." + Environment.NewLine);
}

Some people prefer that, I just am not, and that is up to every coder to decide. If I was to put brackets arround that, I would probably do it this way

else
    { Console.WriteLine("You have entered an invalid date." + Environment.NewLine); }
share|improve this answer
    
I think that even better than NewLine would be to use a separate second Console.WriteLine() for the second newline. –  svick Mar 17 at 22:47
1  
Also, I don't think using 365.25 makes sense here. For example, 700 % 365.25 is 334.75, I don't think that's what you want. –  svick Mar 17 at 22:50
    
3. Your if-else statement looks really weird to me since the if part is using bracket and the else one don't. –  Marc-Andre Mar 18 at 16:48
    
@svick I almost suggested that, but I wanted him to be aware of Environment.Newline. Also, the 365.25 was just to account for leapyear. –  BenVlodgi Mar 18 at 16:52
1  
@Marc-Andre its up to you to bracket how you feel, while they don't match, I think single lines look ugly when surrounded by brackets. If you wanted to you could even put brackets at the beginning and ending of the line. –  BenVlodgi Mar 18 at 16:53
show 2 more comments

I was going to edit my other answer, but instead I'm going to take a different approach - I mean, I'll push it to the extreme ;)


Warning

I'll push it to the extreme is to be taken literally. This solution doesn't aim at solving the simple age calculator problem, rather at showing how one would architect a SOLID application - if the goal is just to calculate the difference between two dates, this is absolute overkill. If the goal is to learn how to write good OOP using a trivial/simple problem as an excuse...

Buckle up, you're in for a ride.


static void Main(string[] args)

As you know by now, this is your application's entry point. When this static method returns, the program ends. To terminate your program, simply use return; in this method, or structure your program flow so that normal exit simply causes the main thread (imagine a cursor running each instruction sequentially - or step through your code in the debugger) to reach the bottom of the Main method.

This method being static, if you're going to call anything outside of it, it's going to have to be static as well. If you're writing procedural code, it doesn't matter.

If you want object-oriented code, the Main method will probably have a very high level of abstraction, and will read like pseudo-code, if not like plain English.

The method revolves around the idea that it's a program's birth, life, and death.

Dependency Injection (DI) disciples (guilty!) call this entry point, the composition root. This is where you instantiate the application (and its dependencies), and run it.

Simplified to the extreme:

static void Main(string[] args)
{
    var app = new MyApplication();
    app.Run();
}

What's in the Run() method?

public class MyApplication
{
    public MyApplication()
    {
        // initialisation here
    }

    public void Run()
    {
        // app logic here
    }
}

Notice that the Run method is not static. It exists only as a member of the interface of an object defined by this MyApplication class - in other words, you need an instance of MyApplication to call this method.

In this case the application logic part will feature our main loop, from which we will exit based on a condition.

public void Run()
{
    var keepRunning = true;

    while(keepRunning)
    {
        // application logic
        keepRunning = false; // exit
    }
}

At this point, we've reached a point of no return. Anything else we code in the Run method will impact maintainability, testability, and readability. Better keep it to a minimum.


The key resides in delegating the work. The MyApplication class cannot do its work alone without using the new keyword, which would increase coupling, and without doing the work all by itself, which would decrease cohesion. Since we want low coupling and high cohesion, we'll start by avoiding the use of static methods and of the new keyword.

Why?

Good code is testable code. You'll want to be able to write tests for the code you write - whether you write these tests or not, writing testable code will tend to produce code that is more cohesive and less coupled.

See also: http://stackoverflow.com/a/3085419/1188513

IUserInteraction

We know we want to use the console to interact with the user, but in order to test our application logic, we'll want to be able to substitute the user's input for whatever our tests need.

public interface IUserInputProvider
{
    string GetUserInput(string prompt);
    T GetUserInput<T>(string prompt, IUserInputValidator<T> validator);
}

With that - and that only, we already have enough to go back to the MyApplication class:

public class MyApplication
{
    private readonly IUserInputProvider _inputProvider;

    public MyApplication(IUserInputProvider inputProvider)
    {
        _inputProvider = inputProvider;
    }

    public void Run()
    {
        var keepRunning = true;

        while(keepRunning)
        {
            var prompt = "Enter your birth date:";
            var input = _inputProvider.GetUserInput(prompt); // no validation for now
            // ...

            keepRunning = false; // exit
        }
    }
}

I'll get back to the IUserInputValidator<T> later.

Mocking

GetUserInput is a method that returns a string. Nothing more, nothing less. We know that we want to call Console.ReadLine(), but that's an implementation detail that the MyApplication class does not need to know about.

If we were to write a test to see if the Run method effectively exits when the user enters "y", we would not bring up a console and wait for someone to enter "y" - instead we would set up a mock - a "fake" implementation of the IUserInputProvider interface that returns "y" when we ask it to GetUserInput.

Implementation

The concrete implementation we're going to be using will use the console. Nothing prevents making another concrete implementation that pops up a dialog window instead - as long as we use a prompt and return a string, anything can work. This means the implementation(s) can be modified in every possible way, the only assumption the MyApplication class makes is that there's a GetUserInput method that takes a string prompt and returns a string.

This could be an implementation:

public class ConsoleUserInputProvider : IUserInputProvider
{
    public string GetUserInput(string prompt)
    {
        Console.WriteLine(prompt);
        return Console.ReadLine();
    }

    public T GetUserInput<T>(string prompt, IUserInputValidator<T> validator)
    {
        string input;
        T result;

        var isValidInput = false;
        while(!isValidInput)
        {
            input = GetUserInput(prompt);
            isValidInput = validator.Validate(input, out result);
        }

        return result;
    }
}

Where IUserInputValidator is yet another abstraction that exposes a bool Validate(string input) method.

Let's make it a generic interface:

public interface IUserInputValidator<T>
{
    bool Validate(string input, out T result);
}

One could implement it like this:

public class BirthDateValidator : IUserInputValidator<DateTime>
{
    public bool Validate(string input, out DateTime result)
    {
        return DateTime.TryParse(input, out result);
    }
}

Or like this:

public enum YesNoResult
{
    Unknown,
    Yes,
    No
}

public class YesNoValidator : IUserInputValidator<YesNoResult>
{
    private readonly IDictionary<string, YesNoResult> _values;

    public YesNoValidator(IDictionary<string, YesNoResult> values)
    {
        _values = values;
    }

    public bool Validate(string input, out YesNoResult result)
    {
        if (string.IsNullOrEmpty(input))
        {
            throw new ArgumentException("input", "input string is empty.");
        }

        var lowerCase = input.Substring(0, 1).ToLower();

        var isValue = values.TryGetValue(lowerCase, out result);
        if (!isValue)
        {
            result = YesNoResult.Unknown;
        }

        return (result != YesNoResult.Unknown);
    }
}

As you can see, this approach produces very focused and specialized code - code that does one thing, so well, that it can't possibly even need to change. And yet it remains extensible - you could decorate any Validator implementation with, say, a ValidationLoggerDecorator that can log all failed validations:

public class ValidationLoggerDecorator<T> : IUserInputValidator<T>
{
    private readonly IUserInputValidator<T> _validator;
    private readonly ILogger _logger;

    public ValidationLoggerDecorator(IUserInputValidator<T> validator, ILogger logger)
    {
        _validator = validator;
        _logger = logger;
    }

    public bool Validate(string input, out T result)
    {
        var isValid = _validator.Validate(input, out result);

        if (!isValid)
        {
            _logger.Info(string.Format("Validation failed for input '{0}'.", input));
        }

        return isValid;
    }
}

Let's step back and take a look at what we've got here:

  • The code remains focused on a single responsibility - following the Single Responsibility Principle.

  • The extensibility depicted with the decorator example, is a side-effect of the Open/Closed principle: a class is closed for modification, open for extension.

  • The fact that the MyApplication class can work with any implementation of the IUserInputProvider interface, regardless of what dependencies that implementation might have, is a side-effect of the Liskov Substitution Principle.

  • Following the Interface Segregation Principle makes our interfaces be very focused as well, ideally exposing only a single member. This point greatly influences cohesion.

  • The fact that all implementations ("concrete" classes) depend on abstractions, and that these dependencies are injected into their constructor, is following the Dependency Inversion Principle*.

Together, these 5 points spell SOLID.


The IUserInputValidator we're passing to the GetUserInput() method, must come from somewhere. But if we create a new BirthDateValidator() our class will be tightly coupled with that specific implementation, and it will become very hard to test the GetUserInput() method and control validation from the outside.

The MyApplication class can receive the validators it needs in its constructor, and we can extract some of the logic from the Run method, into their own private methods:

public class MyApplication
{
    private readonly IUserInputProvider _inputProvider;
    private readonly IUserInputValidator<DateTime> _dateValidator;
    private readonly IUserInputValidator<YesNoResult> _confirmationValidator;

    public MyApplication(IUserInputProvider inputProvider, 
                         IUserInputValidator<DateTime> dateValidator, 
                         IUserInputValidator<YesNoResult> confirmationValidator)
    {
        _inputProvider = inputProvider;
        _dateValidator = dateValidator;
        _confirmationValidator = confirmationValidator;
    }

    public void Run()
    {
        var keepRunning = true;

        while(keepRunning)
        {
            var date = GetBirthDate();
            // ...

            keepRunning = !GetExitConfirmation();
        }
    }

    private DateTime GetBirthDate()
    {
        var prompt = "Enter your birth date:";
        var input = _inputProvider.GetUserInput(prompt, _dateValidator);

        return DateTime.Parse(input);
    }

    private bool GetExitConfirmation()
    {
        var prompt = "Exit (Y|N)?";
        var input = _inputProvider.GetUserInput(confirmPrompt, _confirmationValidator);

        return input == YesNoResult.Yes;
    }
}

Notice how the constructor could easily get bloated with possibly as many validators as there are things we want to get from the user. 3 constructor parameters is probably ok. More than that though, and I would be tempted to extract the validators into their own object, so as to keep the message clear: the MyApplication class needs validators - it doesn't do validation.

Let's extract them anyway to see what we get:

public class UserInputValidation
{
    private readonly IUserInputValidator<DateTime> _dateValidator;
    private readonly IUserInputValidator<YesNoResult> _confirmationValidator;

    public UserInputValidation(IUserInputValidator<DateTime> dateValidator, 
                               IUserInputValidator<YesNoResult> confirmationValidator)
    {
        _dateValidator = dateValidator;
        _confirmationValidator = confirmationValidator
    }

    public IUserInputValidator<DateTime> DateValidator { get { return _dateValidator; } }
    public IUserInputValidator<YesNoResult> ConfirmationValidator { get { return _confirmationValidator; } }
}

Because the actual calculation algorithm would, by itself, be a reason to change, it's best to encapsulate it in its own class.

@svick's answer could be an implementation of some IAgeCalculator interface.

The MyApplication class now looks like this:

public class MyApplication
{
    private readonly IUserInputProvider _inputProvider;
    private readonly IAgeCalculator _calculator;
    private readonly UserInputValidation _validation;

    public MyApplication(IUserInputProvider inputProvider,
                         IAgeCalculator calculator, 
                         UserInputValidation validation)
    {
        _inputProvider = inputProvider;
        _calculator = calculator;
        _validation = validation;
    }

    public void Run()
    {
        var keepRunning = true;

        while(keepRunning)
        {
            var date = GetBirthDate();
            DisplayAge(date);

            keepRunning = !GetExitConfirmation();
        }
    }

    private DateTime GetBirthDate()
    {
        var prompt = "Enter your birth date:"; 
        var input = _inputProvider.GetUserInput(prompt, _validation.DateValidator);

        return DateTime.Parse(input);
    }

    private void DisplayAge(DateTime date)
    {
        var result = _calculator.GetDifferenceInYearsAndDays(date, DateTime.Today);
        var message = string.Format("Your age: {0} years and {1} days", result.Item1, result.Item2);
        _inputProvider.ShowMessage(message);
    }

    private bool GetExitConfirmation()
    {
        var prompt = "Exit (Y|N)?"; 
        var input = _inputProvider.GetUserInput(confirmPrompt, _validation.ConfirmationValidator);

        return input == YesNoResult.Yes;
    }
}

The above assumes a ShowMessage method was added to the IUserInputProvider interface; this method is implemented like this:

public void ShowMessage(string message)
{
    Console.WriteLine(message);
}

Now, the Main method can serve its purpose: compose the application!

static void Main(string[] args)
{
    var input = new ConsoleInputProvider();
    var calculator = new AgeCalculator();
    var logger = LogManager.GetLogger("logger"); // gets a NLog logger
    var dateValidator = new ValidationLoggerDecorator(new BirthDateValidator(), logger);
    var yesNoValues = new Dictionary<string, YesNoResult>
                          {
                              { "y", YesNoResult.Yes },
                              { "n", YesNoResult.No }
                          };
    var confirmationValidator = new YesNoValidator(values);
    var validation = new UserInputValidation(dateValidator, confirmationValidator);

    var app = new MyApplication(input, calculator, validation);
    app.Run();
}

As you can see, inversion of control makes it easy to change our minds and swap the dateValidator for a simple BirthDateValidator, or to write a unit test that will only test how date validation operates, or only how age gets calculated, independently of everything else, without requiring user intervention, and with every dependency under full control.

Of course the composition root for this trivial application is instantiating the objects manually (a.k.a. poor man's DI), and it's quite manageable. For a bigger application, you could leave this daunting task to your favorite IoC container, leaving your Main method looking something like this (here using Ninject):

static void Main(string[] args)
{
    var kernel = new StandardKernel(new MyApplicationNinjectModule());
    var app = kernel.Get<MyApplication>();
    app.Run();
}

The entire application's dependency graph gets resolved in a single method call - Ninject isn't the fastest at that, but its nice syntax and great extensibility make it a solid candidate to consider. If you don't know what a StandardKernel does, the above code might seem automagical if I tell you that it does the exact same thing as the previous snippet.

And that's long enough.

share|improve this answer
    
You call both DateTime.TryParse(input) and DateTime.Parse(input). You also have input.ToLower() == "y" in YesNoValidator and input.ToUpper() == "Y" This is not dry. You really need something like F# Option or use null as return value or something similar. –  abuzittin gillifirca Mar 18 at 9:22
    
Doesn't "ideally exposing only a single member" imply that everything can be declared as a delegate instead of as an interface? For example, instead of IUserInputProvider inputProvider, Func<string,string> GetUserInput (input parameter is the prompt and return value is the user input)? And instead of IUserInputValidator confirmationValidator, Func<string,bool> Validate? With interfaces, you need to declare the interface, declare the method in the interface, declare a separate class which implements the interfaces, construct an instance of the class, ... –  ChrisW Mar 18 at 13:34
1  
Maybe teaching/implementing this using interfaces instead of delegates is a hang-over from Java? –  ChrisW Mar 18 at 13:42
2  
@ChrisW lol, this is overkill in many, many ways; indeed, delegates would be simpler - the idea was to use the OP's code/project as a "small app" to show how one would use these techniques to make a "bigger app" (as explained in Is decoupling necessary for very small applications?). OTOH I do have a tendency to underexploit delegates and overuse interfaces, you have a point :) –  Mat's Mug Mar 18 at 13:48
1  
I'd be inclined to use an interface (or class) instead of a delegate (or method) iff there are two related/codependent method rather than just one. –  ChrisW Mar 18 at 14:17
show 4 more comments

To calculate the date difference properly, you need to take leap years into account. And you need to use the proper rules for deciding when is a leap year. Fortunately, the .Net libraries already know all that.

So, how to do it? Find out the person's last birthday and then calculate the year difference since the birth date and then the day difference until today. In code:

public static Tuple<int, int> GetDifferenceInYearsAndDays(DateTime birth, DateTime today)
{
    if (birth > today)
        throw new InvalidOperationException();

    var thisYearBirthday = new DateTime(today.Year, birth.Month, birth.Day);
    var lastBirthday =
        thisYearBirthday <= today ? thisYearBirthday : thisYearBirthday.AddYears(-1);

    int years = lastBirthday.Year - birth.Year;
    int days = (today - lastBirthday).Days;

    return Tuple.Create(years, days);
}

This might be more complicated than your code, but it's correct, and I also think that it's more readable.

The code is in a separate method, to follow separation of concerns.


One more thing: whenever you use DateTime.Now (or DateTime.Today), you should access that property only once and store that value in a variable, or something. Otherwise, your code might have a really weird bug while everyone else is celebrating New Year.

share|improve this answer
    
I just realized that this won't work if the birth date is on 29 February. Not sure how to fix that, while still keeping the code relatively simple. –  svick Mar 18 at 11:52
1  
Assuming that in a leap year the birthday is on 28 February: thisYearBirthday = new DateTime(today.Year, birth.Month, birth.Day > DateTime.DaysInMonth(today.year, birth.Month) ? DateTime.DaysInMonth(today.year, birth.Month) : birth.Day). But introducing a variable for DaysInMonth would be better. Maybe this would make the code not so simple anymore –  comecme Mar 21 at 12:36
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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