tags:

views:

336

answers:

4

Basically I just want to check if one time period overlaps with another. Null end date means till infinity. Can anyone shorten this for me as its quite hard to read at times. Cheers

    public class TimePeriod
    {
        public DateTime StartDate { get; set; }
        public DateTime? EndDate { get; set; }

        public bool Overlaps(TimePeriod other)
        {
            // Means it overlaps
            if (other.StartDate == this.StartDate
                || other.EndDate == this.StartDate
                || other.StartDate == this.EndDate
                || other.EndDate == this.EndDate)
                return true;

            if(this.StartDate > other.StartDate)
            {
                // Negative
                if (this.EndDate.HasValue)
                {
                    if (this.EndDate.Value < other.StartDate)
                        return true;
                    if (other.EndDate.HasValue && this.EndDate.Value < other.EndDate.Value)
                        return true;
                }

                // Negative
                if (other.EndDate.HasValue)
                {
                    if (other.EndDate.Value > this.StartDate)
                        return true;
                    if (this.EndDate.HasValue && other.EndDate.Value > this.EndDate.Value)
                        return true;
                }
                else
                    return true;
            }
            else if(this.StartDate < other.StartDate)
            {
                // Negative
                if (this.EndDate.HasValue)
                {
                    if (this.EndDate.Value > other.StartDate)
                        return true;
                    if (other.EndDate.HasValue && this.EndDate.Value > other.EndDate.Value)
                        return true;
                }
                else
                    return true;

                // Negative
                if (other.EndDate.HasValue)
                {
                    if (other.EndDate.Value < this.StartDate)
                        return true;
                    if (this.EndDate.HasValue && other.EndDate.Value < this.EndDate.Value)
                        return true;
                }
            }

            return false;
        }
    }
+2  A: 

Check this out: DateTimeOverlaps

Very generally, if all variables are nullable datetimes, then

   return (StartA.HasValue? StartA.Value:DateTime.Minimum) <= 
             (EndB.HasValue? EndB.Value:DateTime.Maximum)  && 
          (EndA.HasValue? EndA.Value:DateTime.Maximum) >= 
              (StartB.HasValue? StartB.Value:DateTime.Minimum);

The concept, (as explained in link) is very simple, and is simply and concisely expressed above.

If the start is before the others end, and the end is after the other start, you have overlap. This says it all, and all that is necessary, in one simple sentence with two clauses, and whatever code you write should concisely map to that simple concept without obfuscating it. Adding extra unecessary complexity does not add understanding, it only adds length.

Fail Case 1: TopStart AFTER other End - Fail

       |----------|
|--|

Fail Case 2: TopEnd AFTER other start - Fail

   |-----------|
                   |------|

In all other cases, start is before other end, and end is after other start.

case A

    |----------|  
|-----|

case B

    | ---------|
|-------------------|

case C

|-----------|
   |------|

case D

   |-----------|
       |-------------|
Charles Bretana
Don't think it handles null end date
Titan
@Titan, this is easily corrected with minor edit...
Charles Bretana
+14  A: 
public bool Overlaps(TimePeriod other)
{
    return (other.StartDate >= StartDate && 
               (EndDate == null || other.StartDate <= EndDate.Value)) ||
           (StartDate >= other.StartDate &&
               (other.EndDate == null || StartDate <= other.EndDate.Value))
}
Adam Robinson
@Adam, Check your algorithm... First comparison must be startdate and enddate, not both startdates. startdate = 5 jan, enddate = 10jan; other.start = 1 Jan, other.enddate = 1 Feb will return false
Charles Bretana
Adam Robinson
@Adam, sorry my mistake, yr algorithm is correct...
Charles Bretana
Factoring out the common code might make it more readable
Duncan
@Duncan: Just out of curiosity, what "common code" are you referring to? I don't see any repeated expressions...
Adam Robinson
Duncan
+11  A: 

How about this one:

public bool Overlaps(TimePeriod other)
{
    bool isOtherEarlier = this.StartDate > other.StartDate;
    TimePeriod earlier = isOtherEarlier  ? other : this;
    TimePeriod later = isOtherEarlier ? this : other;
    return !earlier.EndDate.HasValue || earlier.EndDate > later.StartDate;
}
Philip Daubmeier
Not quite one line like Adams solution, but I think a little more readable. But that is maybe just a matter of taste.
Philip Daubmeier
This also wrong... First comparison must check this.StartDate against other.endDate
Charles Bretana
@Charles: You are also incorrect here.
Adam Robinson
I think the only thing "wrong" with this is that phil chose to assume that dates which touch boundaries are not considered to be overlapping. But that's really just a matter of choice, rather than a matter of right or wrong. Simply change > to >= to go the other way. (The OP went the other way)
Charles
+2  A: 

Any time you're dealing with pure boolean logic, you can distill your algorithm down to a single statement. But don't assume that just because you can, you should. Unless performance is vital, always go for readable code over compact code. (Not that compactness == performance, necessarily)

This is easy to read because it's comprised entirely of single AND expressions, and it's obvious that they all determine a non-overlap:

public bool Overlaps(TimePeriod other)
{
    if (other.EndDate.HasValue && other.EndDate < StartDate)
        return false;

    if (EndDate.HasValue && EndDate < other.StartDate)
        return false;

    if (!EndDate.HasValue && other.EndDate < StartDate)
        return false;

    if (!other.EndDate.HasValue && EndDate < other.StartDate)
        return false;

    return true;
}

Not that the other answers are bad (I like Adam's; his formatting is obviously designed to aid readability). I'm just saying this because it's clear you're a beginner, and I think this is one lesson that isn't heeded enough (I'm guilty). Somebody (I think Martin Fowler) once said something like: "Any fool can write code that a computer understands, but a good programmer can write code that a human understands."

Charles