6
\$\begingroup\$

This Lua code runs in redis. It iterates over some JSON docs, and applies some logic based on the user (if any) making the request. I'm sure there's a lot I can improve here.

local function iterAll(limit)
  local start = 0
  local stop = limit-1
  local ids = redis.call('zrevrange', 'questions-by-latest', start, stop)
  local i = 0
  local function it()
    i = i+1
    if i <= #ids then
      return ids[i]
    elseif i > #ids then
      start = start+limit
      stop = stop+limit
      i = 1
      ids = redis.call('zrevrange', 'questions-by-latest', start, stop)
      if #ids > 0 then
        return ids[i]
      end
    end
  end
  return it
end

local function filter(qid, optUid)
  local q = cjson.decode(redis.call('hget', 'questions', qid))
  if not q['is-active'] then return false end

  if optUid then
    local u = cjson.decode(redis.call('hget', 'users', optUid))
    if #u['ignored-tags'] > 0 then
      for idx, itag in ipairs(u['ignored-tags']) do
        for jdx, qtag in ipairs(q['tags']) do
          if itag == qtag then return false end
        end
      end
    end
  end

  return true
end

local offset = tonumber(ARGV[1])
local limit = tonumber(ARGV[2])

local optUid = false
if #ARGV == 3 then
  optUid = tonumber(ARGV[3])
end

local i = 0
local j = 0
local ids = {}
for qid in iterAll(limit) do
  if filter(qid, optUid) then
    i = i+1
    if i > (offset+limit) then break end
    if i > offset then
      j = j+1
      ids[j] = qid
    end
  end
end

return ids
\$\endgroup\$
2
\$\begingroup\$

Firstly, you've hard-coded all of your key names instead of using the KEYS table for passing them to the script - this is both hard to maintain as well as incompatible with clustering.

Next, consider using MessagePack (cmsgpack in Redis' Lua) instead of JSON when serialiazing/deserializing questions (q) and users (u) - it is leaner and faster.

Also, the jdx, qtag loop looks like a membership test, so it should be replaced with a check against an associative table instead.

Overall, the code appears to be quite complex with multiple nested loops. Without more information on the values' sizes, it is hard to make an educated guess whether this is a viable approach, but intuitively I'd be hesitant about this. Furthermore, it is hard to understand the logic's purpose - it is perfectly possible that there are more efficient ways to do/model what you want/need, but it would be better if you'd provide an overview of the context and requirements before.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.