patternMinor
Iterating over JSON docs
Viewed 0 times
jsonoveriteratingdocs
Problem
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
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 idsSolution
Firstly, you've hard-coded all of your key names instead of using the
Next, consider using MessagePack (
Also, the
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.
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.
Context
StackExchange Code Review Q#27632, answer score: 2
Revisions (0)
No revisions yet.