Parsing and formatting a time two ways (perfectionistic approach)

Question

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;
}

Answer 1

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

Answer 2

I would personally use the first solution. The fact that all assignments are clustered together makes it more readable imo. As well as the fact that it adheres more to the stereotypical organization of method implementations. (Ie: Variable declarations followed by assignments, in seperate blocks)

Solution 1:

+ Typical Declaration -> Implementation statement ordering.
+ Assignments clustered into one codeblock.
+ If - else with braces demonstrate control flow more clearly.

- Uses slightly more space due to braces around if.

Compare to the second solution, where the last 2 assignments depend on the first 2 assignments, but those are in their seperate 'block' of statements.

Solution 2:

+ Theoretically shorter due to lack of braces.

- Cluttered spacing of statements. (You've got 4 distinct codeblocks, block 4 depends on block 3 but is not grouped with it)
- Assignments / declarations Mixed. (Why isn't TimeSpan eventTime located at the top?);
- The single line-if statement slightly hampers readability. (Important in this case because the return exits the function.)

So the flow of the function is easier to read in solution 1 as well if you ask me, it's instantly clear what happens where. Whereas in solution 2 I have to realize that the single line if statement actually returns to the calling function.

Answer 3

Well, there's a few issues present for both versions:

• eventDateWithTime: "DateTime" is commonly enough used terminology that this should just be called eventDateTime. That already separates it out sufficiently from EventDate or eventTime.
• EndTime is apparently a string, whereas EndDate is a DateTime. While I'd generally advise against variable naming along the lines of Hungarian notation (where the type is specified in the variable name), you also don't want to pick a name that has misleading implications about the type. It's probably best to name this field what it's actually for, like EndTimeDisplay or EndTimeText.
• TryParse. Is this actually appropriate? It's hard to guess without seeing more of your code, but unparseable dates may be something you'd consider exceptional. In that case it's better to let Parse throw its exception, then handle it in the appropriate place. Again, this may not be the case, you'll have to make that judgement based on the wider code.
• Unnecessary method-local variables. eventDateWithTime and, in the second example, eventTimeString can both be removed by assigning directly to the class-level variables. Potentially you could argue that this is less clear, because the local variables state what those variables represent within that method. But it's not actually clear to me that that's the case- do those variables actually represent anything different in the method vs the full class? Again, not a judgement I can make perfectly without seeing the full code, but consider that they may not be needed.

Now to your specific approaches. With the second one, I find this quite difficult to read:

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

This is somewhat a matter of taste, but I know I'm not the only one. Generally I'd only put both parts in-line without braces if they're both very short. Putting the return statement on a new line without braces is generally considered poor practice so I won't recommend it, though it is actually my favourite in terms of readability alone. So two remaining options are:

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

or

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

Of those, the first would be my preference.

In terms of choosing approaches, with the above changes made I would prefer the second. In my opinion, returning early makes code easier to read because, mentally, you can completely discount paths through the code which have already returned. Having to only temporarily make that mental switch as you enter an if statement means having to maintain a more complex mental "stack" of possible paths through the method. True, you could scroll all the way to end end of the if statement and see that there's no more code afterwards, but it's much nicer to be able to read code start-to-finish, rather than jumping back and forth.