facets
facets copied to clipboard
rubyworks
Add Array#to_ranges
I needed this code for a project and figured I'd offer it up to facets
I originally found this code at https://dzone.com/articles/convert-ruby-array-ranges but it didn't actually work as described, so I fixed it.
Looks good to me. But the tests didn't run for some reason. I don't think it's your code. I'll look into it and get back to you.
It's odd that travis blew up.
Can we make self.compact.uniq.sort better?
Compact is O(N). Uniq might be O(N^2) search? sort is okay.
However, it's also making 3 copies of the array for no reason. Could be even self.dup.sort!.uniq!.compact! or something like that.
if !array.empty? AFTER you already did compact.uniq.sort seems inefficient. It could be the first line.
return self if self.empty?
Well, it depends on the assumptions you want to make, but I prefer not doing additional allocations.
^_^ > irb
2.3.0 :001 > x = []
=> []
2.3.0 :002 > x.object_id
=> 70296120613840
2.3.0 :003 > x.sort.object_id
=> 70296111721840
As an example, sort returns a new array, so it supports your argument.
In fact something like:
def to_ranges
ranges = []
return ranges if empty?
array = uniq.compact
return ranges if array.empty?
array.sort!
...
And get rid of the if !array.empty? clause altogether.
While we are on the topic of bike shedding lol, perhaps compact is unnecessary. Because it should be a contract of the caller to provide the right kind of array?
Anyway if it can be improved with the above advice I think it's a good addition.
Well, it depends on the assumptions you want to make, but I prefer not doing additional allocations.
In this case you really have to because if you return self they could modify it in place,
f = [1,2,3]
g = f.to_ranges
g << (4..5)
f => [1..3,4..5]
And that's not what would be expected.
While we are on the topic of bike shedding lol, perhaps compact is unnecessary. Because it should be a contract of the caller to provide the right kind of array?
I suspect that the algorithm won't do what one expects then. I have to try it, but I expect we'd get things like,
[1,2,2,3,3].to_ranges #=> [1..2,2..2,3..3]
And I don't think that would ever be what anyone wants. That being so the compact is really an integral part of the behavior.
@aguynamedryan What do you think?
Hey I just rediscovered that Facets already has this, arrange. So the task now becomes to compare implementations, and improve the code accordingly. Also, I think we need to ask if the name "arrange" is really the right one. Perhaps at the very least, we could alias to_ranges -- but is it good practice to call a method "to_foo" when there is no Foo class?
Well, there is a Range class. to_ranges is just a plural.. so it makes sense in a way.
@ioquatix You know, you are right. I'll change it to #to_ranges and figure out the best implementation between this and the current #arrange.
Someone remind me to do this if I haven't gotten around to it within the next few weeks.