Skip to main content
added 5 characters in body
Source Link
Ben Aaronson
  • 5.7k
  • 21
  • 39

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"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.

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 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.

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.

Source Link
Ben Aaronson
  • 5.7k
  • 21
  • 39

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 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.