Enumerating new articles whose publication start date has passed

Question

(cn.PublishEnd == null || cn.PublishEnd < DateTime.Now)

vs

(cn.PublishEnd ?? DateTime.MinValue < DateTime.Now)

Which is more readable? I'm inclined towards the second form, but something tells me I'm wrong.

Context:

namespace Damnation.Website.Main.Business.Extensions
{
 public static class CommunityNews
 {
 public static IEnumerable<DAL.CommunityNews> Published(this ObjectSet<DAL.CommunityNews> table)
 {
 return table.Where(cn => cn.PublishStart > DateTime.Now && DateTime.Now > (cn.PublishEnd ?? DateTime.MinValue)).OrderByDescending(cn => cn.PublishStart);
 }
 }
}

Types

DateTime PublishStart
DateTime? PublishEnd

• Jeff Mercado: In light of seeing Ivan's answer, I think we'll need a bit more context like, what's that actual type of cn.PublishEnd? If it happened to be of type bool? (very odd), then that would invalidate my answer and would need clarification. Is this going to be a part of a conditional and you use the value somewhere? Other things will change as well in that case.
• bevacqua: It's DateTime?, I'll update with the full snippet.

Answer 1

As for me, the first form is more readable because it clearly describe the condition. It's easy to read and understand. The second form is a little confusing. The first thought I had was: What is the DateTime.Zero? Is this a default value for PublishEnd? If yes then why not to use the DateTime.MinValue? Maybe it's not a default value and I get it wrong somewhere? Of course these thoughts took only seconds but nevertheless. In general I think that ?? should be used to return the default value with expected type. The default value of the PublishEnd should be of DateTime type. But the second form distorts the meaning of ?? and returns bool type instead, and it’s confusing.

Edited: If you want to read is like you said: "if now is greater than (end date or earliest date)" then you should write it like:

(DateTime.Now > cn.PublishEnd ?? DateTime.MinValue)

And this, third form is better than first and second.

• bevacqua: it's DateTime.MinValue, I mistyped it. I find it easy to read as "if now is greater than (end date or earliest date)"
• Ivan: Yes, this part is easy to read, but for me it's confusing because it is not plainly connected to PablishEnd. It's connected to PablishEnd by means of operator that has specific purpose. Once again - it's only my opinion. I prefere plain code.
• Jeff Mercado: Hmm, I didn't realize there wasn't a Zero property of Datetime. Hopefully he intended to use the .NET DateTime and not some custom class, it'd be bad form to name a type the same as one in the System namespace. Also good point at the end, I forgot that the ?? operator has lower precedence than comparison operators. But the intent is clear, any decent IDE would help correct that rather quickly.

Answer 2

Within a conditional, it would depend largely on the type of the object. In general, I'd favor the first form, particularly if it is a non-string reference type that doesn't offer a nice default value. Otherwise if it was a string or nullable structure, I would prefer the second.

I'd be very careful instantiating new objects only for doing a comparisons like this. It's rather wasteful, especially if you have many comparisons and the object isn't very cheap to instantiate since it's being thrown away.

In this case, it is a nullable (DateTime? apparently) so it would be cleanest IMHO using the second.

Answer 3

Usualy I solve problems like this with introducing a class DateTimeRange which has a method Contains(DateTime date).

In your case I would rewrite the conditional statement and put it into the CommunityNews class

public class CommunityNews
{
 public bool IsPublished(DateTime checkingDate)
 {
 if (this.PublishStart <= checkingDate)
 {
 return false;
 }
 return this.PublishEnd.HasValue
 ? checkingDate < this.PublishEnd.Value
 : true;
 }
}

Then your search method would become as clear as possible:

 public static IEnumerable<DAL.CommunityNews> Published(this ObjectSet<DAL.CommunityNews> table)
 {
 return table.Where(cn => cn.IsPublished(DateTime.Now)).OrderByDescending(cn => cn.PublishStart);
 }

P.S. In general I would consider using cn.PublishEnd ?? DateTime.MinValue < DateTime.Now in a check statement as a bad practice, because to me ?? is more about return and not about checking.