Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update overlaps to ignore end date #14

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

mwptrsn
Copy link

@mwptrsn mwptrsn commented Aug 18, 2014

Story

https://www.pivotaltracker.com/s/projects/912050/stories/77112746

Notes

If both object and other have non-null end dates, behavior is as previously defined. Otherwise, only look for a start before start.

@cmoesel
Copy link

cmoesel commented Aug 18, 2014

Two comments:

(1) The QDM User Group is scheduled to discuss this issue on 8/20/2014. I expect they will confirm that this is the behavior they want, but we should wait to see before merging.

(2) This particular implementation is logically incorrect. Here is a simple test case that should evaluate to false but would evaluate to true using this logic:

A overlaps B
A.start = 1/1/2014 A.end = 2/1/2014
B.start = 6/1/2014 B.end = null

I believe that a proper implementation probably needs to have different logic depending on whether A's end date is null or B's end date is null.

@mwptrsn
Copy link
Author

mwptrsn commented Aug 18, 2014

Thanks for pointing that out. I added a test to cover that case, and adjusted the logic.

The only question I had, and this is probably a philisophical one to be decided by the QDM working group, is what to do if both cases have an undefined end? From the standpoint of a timeline, these would always overlap, so I have that set to true, but I'm not sure if that's the desired behavior. Either way, that would be a simple change.

@cmoesel
Copy link

cmoesel commented Aug 18, 2014

I think that perhaps there are still some issues... I'll go through them one at a time:


if @high.date && other.high.date == null
  true # If neither have ends, they inherently overlap on the timeline

Based on the code comment (and your github comment), I think you mean to check that both are null. The code should probably be:

if @high.date == null && other.high.date == null
  true # If neither have ends, they inherently overlap on the timeline

else if @high.date == null
  this.SBS(other)

In this case we now know that other.high.date exists. So the source event actually overlaps the other if it starts before or during the other (it can overlap on the left or right side of other). The code should probably be:

else if @high.date == null
  this.SBS(other) || this.SDU(other)

But then... I think we can simplify it further, because we're really just saying that the source event can't start after the other event:

else if @high.date == null
  !this.SAE(other)

else if other.high.date == null
  this.SBS(other) && !this.EBS(other)

This would capture an overlap on the left side of other, but wouldn't capture an overlap where the source event starts after other (which is still overlapping since other goes on forever). We actually don't care if the source event starts before other or not (as long as it doesn't end before other), so we can just say:

else if other.high.date == null
  !this.EBS(other)

The rest looks good! Let me know if you have any questions or if I got something wrong...

@mwptrsn
Copy link
Author

mwptrsn commented Aug 19, 2014

Thanks Chris - the first issue was actually taken care of with one of the later commits (345bc7b). I agree on the other points, and will make those changes (and add associated tests). Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants