Skip to content

Instantly share code, notes, and snippets.

@catwell
Created April 13, 2011 16:12
Show Gist options
  • Save catwell/917844 to your computer and use it in GitHub Desktop.
Save catwell/917844 to your computer and use it in GitHub Desktop.
Let's try to refactor info_string, keeping it functional and short
# eg.: @slots = Hash[[5,19,20,32,21,28].zip([:new]*6)]
def info_string
"#{self.to_s.ljust(25)} slots:" + @slots.keys.sort.reduce([]){|a,b|
if a.empty? || !(a[-1][1]+1 == b)
a << [b,b]
else
a[-1][1] = b
a
end
}.map{|x| (x[0] == x[1]) ? x[1] : "#{x[0]}-#{x[1]}"}.join(",")
end
# another style...
def info_string
nums = @slots.keys.sort
start = stop = nums.first
slots = []
(nums[1..-1]+[nums.last+2]).each do |x|
if x == stop + 1
stop += 1
else
slots << ((start == stop) ? start : "#{start}-#{stop}")
start = stop = x
end
end
"#{self.to_s.ljust(25)} slots:" + slots.join(",")
end
@raggi
Copy link

raggi commented Apr 13, 2011

in order to duck type, it's preffered to use x when assigning to stop, rather than += 1, that way you can preserve the type, and trust the equality method. minor, but there we go.

i'm not sure about the wrangling of the slice on the array, it's potentially a little harder to grok than assignment, but that's pure preference. I'd likely make that an instance variable rather than have 8 expressions on one line with carry-forward semantics (the each).

Other than that, this may be more correct than my version, as I didn't test mine at all, i simply refactored.

Your comment that I missed the last one is totally accurate, I'd probably do something like this (again untested, rushing, sorry):

def info_string
  slots, min, max = [], nil, nil
  keys = @slots.keys.sort
  # The last 'key' is a ghost to cause concatenation.
  # Assumes that (nil == max + 1) is not true
  keys << nil
  keys.each do |key|
    min ||= key
    max ||= key
    if key == max + 1
      max = key
    else
      slots << min == max ? min.to_s : "#{min}-#{max}"
      min, max = nil
    end
  end
  "#{self.to_s.ljust(25)} slots:#{slots.join(',')}"  
end

References:

https://github.com/antirez/redis/blob/unstable/src/redis-trib.rb#L78
https://github.com/antirez/redis/blob/unstable/src/cluster.c#L1069

And a twitter conversation...

@catwell
Copy link
Author

catwell commented Apr 14, 2011

This code still doesn't work. I would say that's because although it looks simpler than mine it is actually more complicated.

I prepared the array and made sure I only used integers whereas you chose to use nil and test for it at every step. This makes more operations, but that's not the problem.

  1. At the end of every step, if key != max + 1, you set max and min to nil. Then at the beginning of each step you set min and max to key. So if they've been set to nil they will be set to key which is an integer, right?

Hmm... what's the last value key will take? ;)

  1. There's a logic problem. You're setting min, max = nil at the end of an iteration and then to key at the beginning of the next one. But it's not the same key anymore...

So what you'd need is min = max = key. But then you have a problem with the first key, which can be added twice. So you need to skip the first iteration (hello, array slicing :p).

  1. I had gotten 1) and 2) right but I actually tested my code and I had done a similar mistake, because this is unintuitive:
min = 1; max = 2; slots = []
slots << min == max ? min.to_s : "#{min}-#{max}" # yours
slots << (min == max) ? min.to_s : "#{min}-#{max}" # my 1st try
slots << ((min == max) ? min.to_s : "#{min}-#{max}") # mine above

Quizz: what's in slots now?

Answer:
[1, false, "1-2"]

When in doubt, use parentheses ;)

@raggi
Copy link

raggi commented Apr 14, 2011

Fair answer - that's what I get for throwing together something in a rush and not testing it :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment