Simple rate limiting class in Ruby

Question

The idea of this class is I would be able to limit the use of an API/method by sending it a key which would be an IP address or username and it would tell me if they are under the rate limit.

require 'time'

class RateLimit
    def initialize(time, limit)
        @reset_on = Time.now + time
        @limit = limit
        @keys = {}
    end

    def increment(key)
        reset if Time.now >= @reset_on

        if @keys[key] == nil
            @keys[key] = 1
        else
            @keys[key] += 1
        end
        return @keys[key] >= @limit
    end

    def current_level(key)
        reset if Time.now >= @reset_on
        return @keys[key]
    end

    def is_limited?(key)
        reset if Time.now >= @reset_on
        return @keys[key] != nil && @keys[key] >= @limit
    end

    def reset
        @reset_on = Time.now + @limit
        @keys = {}
    end
end

To me this seems like some pretty clean code but are there any issues I have missed or things that could be done better?

Answer 1

So there is a flaw in this approach. It limits requests based on the current time, and not by how many requests happened in the last amount of time. Let me give you an example. We'll start at time 0 with a limit of 10 requests per every seconds.

Time | Requests (total)
--------------
0    |     0
1    |     0
2    |     0
3    |     0
4    |     0
5    |     0
6    |     0
7    |     1
8    |     3
9    |     10 # requests start getting limited
10   |     10
# your request tracking resets here and requests are no longer limited
11   |     15
12   |     18
13   |     20
14   |     20
...

Do you see the issue? Between seconds 9 to 13 (4 seconds), we had a total of 17 requests when only want to allow 10 requests per every 10 seconds.

This shows that the initial approach is flawed.

In terms of code quality, I see a few things that could be improved:

1) Variable and method names:

@keys = {} is not immediately clear what it holds. However, if it were called requests_per_ip_address I would know. Long variable names better if they describe what they're holding.

is_limited? could just be limited?

2) Working with nil you should use #nil?

For example: @keys[key] == nil could be @keys.nil? and @keys[key] != nil could be !@keys[key].nil?

3) Don't return at the end of methods. The last line or branch of a method is what gets returned. The explicit return is not necessary.

4) Use private. Anyone can call any method on your rate limiter, but we only want a couple of methods to be public.

5) Use a linter. Check out RuboCop. You will be surprised by all of the style rules you break!

Here is an example of how I might write this class:

class RateLimit
  def initialize(maximum_requests, time_limit)
    @maximum_requests = maximum_requests
    @time_limit = time_limit
    @user_requests = Hash.new { |h, k| h[k] = [] }
  end

  def increment(ip_address)
    return false unless user_under_limit? ip_address
    @user_requests[ip_address] << Time.now
    true
  end

  def user_under_limit?(ip_address)
    remove_first_expired_request ip_address
    @user_requests[ip_address].count < @maximum_requests
  end

  private

  def remove_first_expired_request(ip_address)
    # We can just remove the first request if it is expired
    # and that will make room for a new request
    if @user_requests[ip_address].first < Time.now - @time_limit }
      @user_requests[ip_address].shift 
    end
  end
end