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

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.

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.