HiveBrain v1.2.0
Get Started
← Back to all entries
principlecsharpMinor

Parsing and formatting a time two ways (perfectionistic approach)

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
formattingperfectionisticwaystimetwoparsingandapproach

Problem

I'm still not satisfied by the readability of the code. The wanted behavior which would be self descriptive would sound like: "I've two fields; one is for the date and one for the hour. This two fields should be in a UserControl of which I have two instances, one is for the event time and one is for the event end time."

My personal opinion is all the extra code is something which is specific to C# syntax, and not to my purpose. Handling null, exceptions, conversions between dates and strings makes me to write that extra code and a developer could get confused and wonder on what was the purpose of that solution. The only thing which is at a functional level in this code is an assignment.

The String.Format is an example of what I mean... it's a really mean solution. Now that every approach tries to separate Model, View and Controllers, I'm mixing the solution I found to to fill a gap of conversion and the code wrote to make the behavior I needed.

Which solution would you use?

First formulation

public override void OnEventTimeChanged()
{
    DateTime eventDateWithTime;
    TimeSpan eventTime;

    if (TimeSpan.TryParse(EventTime, out eventTime))
    {
        eventDateWithTime = (EventDate.Date + eventTime);
        CultureInfo cultureInfo = Thread.CurrentThread.CurrentUICulture;
        EndTime = String.Format(cultureInfo, "{0:HH:mm}", eventDateWithTime);
        EndDate = eventDateWithTime;
    }
}


Second formulation

public override void OnEventTimeChanged()
{   
    CultureInfo cultureInfo = Thread.CurrentThread.CurrentUICulture;

    TimeSpan eventTime;
    if (!TimeSpan.TryParse(EventTime, out eventTime)) return;

    DateTime eventDateWithTime = (EventDate.Date + eventTime);
    string eventTimeString = String.Format(cultureInfo, "{0:HH:mm}", eventDateWithTime);

    EndTime = eventTimeString;
    EndDate = eventDateWithTime;
}

Solution

In the 2nd version, the if acts as a guard: it prevents invalid states to get through it. Many people prefer it this way, for example in this related post:
Should I return from a function early or use an if statement?

Which way you choose, is a matter of taste. There's nothing wrong with either one.

If you do go with the 2nd one, it can be improved a little bit:

public override void OnEventTimeChanged()
{   
    TimeSpan eventTime;
    if (!TimeSpan.TryParse(EventTime, out eventTime))
    {
        return;
    }

    DateTime eventDateWithTime = EventDate.Date + eventTime;
    CultureInfo cultureInfo = Thread.CurrentThread.CurrentUICulture;    
    string eventTimeString = String.Format(cultureInfo, "{0:HH:mm}", eventDateWithTime);

    EndTime = eventTimeString;
    EndDate = eventDateWithTime;
}


That is:

  • cultureInfo is not needed in the if, so declare and initialize it later. Even more precisely, declare and initialize it right before you really need it.



  • It's recommended to use braces with if statements always, even if only one line will go inside the block. Remember this bug: https://www.imperialviolet.org/2014/02/22/applebug.html

Code Snippets

public override void OnEventTimeChanged()
{   
    TimeSpan eventTime;
    if (!TimeSpan.TryParse(EventTime, out eventTime))
    {
        return;
    }

    DateTime eventDateWithTime = EventDate.Date + eventTime;
    CultureInfo cultureInfo = Thread.CurrentThread.CurrentUICulture;    
    string eventTimeString = String.Format(cultureInfo, "{0:HH:mm}", eventDateWithTime);

    EndTime = eventTimeString;
    EndDate = eventDateWithTime;
}

Context

StackExchange Code Review Q#63030, answer score: 4

Revisions (0)

No revisions yet.