ivx / time_frame

Provides an easy to use time frame class for Ruby
11 stars 4 forks source link

Operator TimeFrame#& should not return nil #1

Closed mblumtritt closed 10 years ago

mblumtritt commented 10 years ago

The result of an operator has to be bijective. If there is no way to return the expected type (TimeFrame) you should rather throw an exception.

pderichs commented 10 years ago

Can you give an example where the operator does not behave as expected? Where is the type issue? Do you mean the nil result in case of no intersection?

mblumtritt commented 10 years ago

Yes, I mean the return of nil. That's unexpected and may result in follow-up errors.

pderichs commented 10 years ago

We discussed the return value in case of non intersecting TimeFrame instances too, and we decided to go with the nil solution.

What would you expect to be returned if there is no intersection? An empty TimeFrame instance? Which time value for min should it contain?

It would be possible to define a time value like 01.01.1970 to be the indicator for these situations but IMO that would not be a nice solution.

Alternative solution: We could also create an instance of TimeFrame within the gem, which is empty by default and which would be returned in every case where an empty range should be returned - let's discuss about that.

In terms of code:

Current solution:

time_frame = TimeFrame.new(min: Time.new(2014), duration: 1.day)
other_time_frame = TimeFrame.new(min: Time.now, duration: 1.hour)
intersection = time_frame & other_time_frame
puts 'Intersecting!' if intersection

The empty TimeFrame solution:

time_frame = TimeFrame.new(min: Time.new(2014), duration: 1.day)
other_time_frame = TimeFrame.new(min: Time.now, duration: 1.hour)
intersection = time_frame & other_time_frame
puts 'Intersecting!' unless intersection.empty?

I like the first code sample a bit more, but this is a matter of personal taste - I am happy with both solutions in the end :wink:

bstoecker commented 10 years ago

First of all: Coming from a mathematical background I have to say that this operator can never be bijective since the preimage is a space of dimension 2 and the image a space of dimension 1. So it is easy to generate four completely different timeframes t1 to t4 where t1 & t2 = t3 & t4. That means this operator is not injective and so also not bijective.

Lets get back to the topic. While having to timeframes t1 and t2 as imput we can have two scenarios: they cover a common timeframe or they don't. in case they cover a common timeframe, I think you agree with the result. So lets talk about the scenario they don't have a common timeframe. If they do not have a common timeframe, the result is (looking on it from a mathematical perspective with intervals) the emptyset (or nullset). So we decided to return nil which is the mathematical representative for empty set.

But lets discuss this approach. You suggest to throw an error, but from our current point from view this makes it a bit more difficult to handle it in code since you have to try catch (begin/rescue) it and (and this is more important to me) it's mathematically wrong, since the result is just the empty set. I think the only alternative which makes sence is a timeframe with no data. Such a timeframe should not have a min or a max value (timeframe.min = timeframe.max = nil). But such a change might also have some other strange impacts. What is the duration of such a timeframe? nil? 0? What is its behavior regarding the other methods (#without, #deviation_of...).

bstoecker commented 10 years ago

I also change the topic header, since this title is not correct

jzernisch commented 10 years ago

I agree with @mblumtritt, as other Ruby core/stdlib classes like Array and Set have null objects, we should be consistent with that. An empty TimeFrame should then return nil for #min and #max as this is also the standard behavior for Enumerable types. The only method that's non-trivial to implement would be #deviation_of (but when I think about it I'm not sure whether we need that method in TimeFrame at all).

bstoecker commented 10 years ago

we already used the #deviation_of method within one of our apps. in case of an empty time_frame I would suggest just to raise an error, since the reponse would be undefined. to implement an empty timeframe, we have two possibilities:

  1. the first one is to add a flag. this would be the short, but dirty solution which I don't prefer, since we would lose maybe our min/max validation.
  2. the second solution could be an empty timeframe class which behaves as expected. This kind of timeframe should usually not be initialized by a user (which would not make much sense). This is a solution which might be better. what are you thinking about that?
jzernisch commented 10 years ago

@bstoecker: Not sure whether I understand what you mean with (1). I'd prefer a simple null object similar to Array or Set. This object could be held in a constant like TimeFrame::Empty or something like that, so one could reference it, but it wouldn't be constructible via .new (although it's an instance of TimeFrame, of course). I guess this is also what you mean with (2), right? Regarding the #deviation_of method: Actually it's just the Hausdorff Distance, so we could just use its canonical extension for empty sets (distance between two empty sets is 0, distance between a non-empty set and the empty set is positive infinity).

pderichs commented 10 years ago

So we need an empty TimeFrame.

I would suggest to enhance the TimeFrame class so it can deal with empty TimeFrame instances correctly, means: handling the case of min and max with nil values correctly within the class.

I would suggest to provide an constructor which can handle no params which would lead to an empty TimeFrame instance by default (we are now forcing min, max or duration to be present). So the bound checks can stay for the case if min, max or duration are present in the params hash.

I don't consider #deviation_of to be useless - but this is another topic. For now I would suggest to throw an error if #deviation_of is called for an empty TimeFrame instance (it would make no sense to call it and it would be the easiest solution).

bstoecker commented 10 years ago

regarding #deviation_of I am with you @jzernisch. I think this would be a good solution.

pderichs commented 10 years ago

@jzernisch Can you clarify the meaning of "I'd prefer a simple null object similar to Array or Set." ? Do you mean Null Objects like https://github.com/avdi/naught or do you mean something else?

bstoecker commented 10 years ago

in case of an empty-timeframe object we should also consider to change the behavior of #empty?. Currently every timeframe having no interior points is empty. maybe the method #empty? should only return true, when we use our "empty frame object". the functionality of the current #empty?method can be handled within a method #interior?.

jzernisch commented 10 years ago

@pderichs What I mean was that we have a TimeFrame object that represents the empty time frame in the sense that it responds to alle TimeFrame methods in a reasonable way. I'm not sure whether such an object should be constructible or not and how one would implement it. Dropping the requirement of lower and upper bounds as you proposed would be possible. @bstoecker I agree with you, although I find a method named "interior" a bit too mathy (I wonder whether it's clear to non-math people what that means).

Does handling all this set-theoretic stuff in a TimeFrame class actually make sense? I mean we're talking about empty sets, intersections, unions and so on -- nothing of which is specific to time. Wouldn't it make sense to outsource that logic into a separate gem (time_frame would then be depending on)?