3
\$\begingroup\$

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;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I usually prefer the way you used the if-statement in the first formulation. It makes it more clear what you're trying to do, instead of that one line that makes the function return if your condition is not met. Aside from that, my previous two employers used code conventions that allowed only one return in a method, so that would make the second formulation impossible to use. I'm not sure if that's a common convention, though. \$\endgroup\$ – Ivo Coumans Sep 16 '14 at 14:01
  • \$\begingroup\$ @IvoCoumans: I also normally use only one return point since it helps with refactoring. It's more annoying to refactor a piece of code where you have a return. You cannot use the "Extract to new method" refactoring. \$\endgroup\$ – Revious Sep 16 '14 at 14:04
  • \$\begingroup\$ Imagine I'm the kind of person that will go through the code really fast a first time to understand the big picture, I will probably not notice the !in your TryParse at a first glance. (I don't think this is worth an answer, so here's a comment) \$\endgroup\$ – IEatBagels Sep 16 '14 at 14:49
4
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ In if-return constructs, it is very common to leave out the curly braces. IMHO, that type of construct is easy to see and understand without cluttering it up with additional noisy lines of code. \$\endgroup\$ – stephenbayer Sep 16 '14 at 16:40
  • \$\begingroup\$ @stephenbayer For the record, I like short code. Fewer lines, fewer symbols. I reluctantly started following the recommendation, and eventually I got used to it. It doesn't bother me anymore. It helped that I've witnessed myself the lack of braces causing nasty bugs in real life. It was similar to this, but less ridiculous: imperialviolet.org/2014/02/22/applebug.html \$\endgroup\$ – Stop ongoing harm to Monica Sep 16 '14 at 16:59
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks, I upvoted your answer (even if I prefer the second one) it's really good to compare different approaches! \$\endgroup\$ – Revious Sep 16 '14 at 14:01
  • \$\begingroup\$ @Revious The question, though, is: why do you prefer the second one? I will admit that I'm fond of single-line if statements if there's no else involved. But overall I think the first one is slightly better structured. Is there anything specific you like in Solution 2 better than in Solution 1? \$\endgroup\$ – ZeroStatic Sep 16 '14 at 14:09
  • \$\begingroup\$ I can try to explain my point of view but it's strictly personal of course. Take it for just for it. In the first approach I have to read every Line and think: "what do he want to do?". For example he declares a DateTime called eventDateWithTime.. My first tought is: "why?". And then I see something about cultureinfo.. and again I have to wonder why the solution he used required to access culture. The only wanted operation was: "EndDate = EvenDate; EndTime = EndDate.ToHour();" this is what I would like to realize immediately at first glance without any effort. \$\endgroup\$ – Revious Sep 16 '14 at 15:02
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ IMHO, if opening braces are always placed either alone on a line or on the same line as the close-brace, and if the nature of the if is that it makes sense for it to do exactly one thing (as in the case of a return, when there's nothing sensible that can be done before it and obviously nothing at all that can be done after it) then I would consider it proper to have a single if-controlled statement on a line by itself without a brace-pair. I would not consider such formatting proper when using a brace style that puts an opening braces on the same line as the preceding statement. \$\endgroup\$ – supercat Sep 16 '14 at 18:52
  • \$\begingroup\$ Essentially, I see putting the open brace on a line by itself as a convention which costs a line of vertical space when a control statement includes multiple lines, but saves one in the case where an if or for controls a single thing. I don't always omit the braces for if statements which have a single consequent action, but base the decision on whether I would view the statement as controlling a sequence of operations that happens to only contain a single item, versus controlling a single operation. \$\endgroup\$ – supercat Sep 16 '14 at 18:56

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.