Scripting Helpers is winding down operations and is now read-only. More info→
Ad
Log in to vote
0

Is there any way to clean this code up? #MakeCodePretty

Asked by 6 years ago

So, this function is meant to shuffle around songs in another array that is more effective than something like

result = math.random(1,#array)
return result

This is what I came up with.. It seems this works, but I need something that looks cleaner and is a bit more effective than this. Any ideas?

local shuffleSongs = function(Array, Amount)
    local i = 0
    local cloned = {unpack(Array)}
    if #cloned then
        return
    end
    if i > Amount then
        return
    end
    i = i + 1
    local Index = math.random(#cloned)
    local Val = cloned[Index]
    table.remove(cloned, Index)
    return i,Index,Val
    --[[
        for i,k,v in next,shuffleSongs do 
            print(k, v) 
        end
    --]]
end

1 answer

Log in to vote
1
Answered by 6 years ago

It looks like you're trying to create an iterator out of this? I have no idea how you've got it to run (or how you think that "It seems to work", unless you didn't test it)

  • #cloned will always evaluate to true because cloned will always be a table, #someTable will always be a number, and numbers always evaluate to true
  • i is always 0 or 1 and will never be greater than Amount (except that if you use it in an iterator, Index will be passed in for Amount), meaning that if you manage to make it run more than once (ex in a loop), there's nothing stopping it from running forever (and there's nothing stopping from it returning the same element multiple times)

In terms of prettiness:

  • Make all your variables lowerCamelCase if you want to be consistent with how Roblox does it (UpperCamelCase is reasonable if it's a public field, like "self.Value"). Some lua styles suggest using "all_lower_case" or "alllowercase" (though I think the latter is hard to read in most cases). Either way, be consistent!
  • If you don't need the index (and I don't know why you would), you can simplify lines 11-14 with return i, table.remove(cloned[Index]), since table.remove returns the removed element (though first assigning it to a variable is arguably more "pretty")
  • Comments or documenting how to call/use the function would also be pretty

Your algorithm needs more work before it'll do anything. Instead, I recommend this simple function, at least as a starting point:

function Shuffle(list)
    local index
    for i = 1, #list-1 do
        index = math.random(i, #list)
        list[i], list[index] = list[index], list[i]
    end
end

It modifies the original list (but doesn't remove anything); you can still clone the list and shuffle that one if you prefer.

If you wanted an iterator, the simplest solution is to use that Shuffle function and then iterate over the first 3 elements with a simple for i = 1, 3 do. If you really wanted an iterator, here's one that stops after the desired number of elements (saving time if you only want 5 random elements out of a list of 1000s)

function nextrandom(list, amount)
    local i = 1
    local index, v
    return function()
        if i > amount or i > #list then return end
        index = math.random(i, #list)
        v = list[index]
        list[i], list[index] = v, list[i]
        i = i + 1
        return v
    end
end
for v in nextrandom({"a", "b", "c"}, 2) do
    print(v)
end

Hopefully the above gives you a demonstration of the syntax required to make it work.

Ad

Answer this question