Better way to architect this function?

Question

I have a function that consists mainly of a massive amount of calls (50+) to another function which inserts data into an array, with logic here and there dictating various conditions to insert various items into the array (plus a little bit on the end that writes the contents of the array out to a file). I'm wondering if there isn't a better way to make this function; I suppose i could start with logically splitting array insertion command sets into their own functions, but I'm wondering if there's any more I can do. Is there?

Example:

function buildTable(fileName, data)
    local dataToWrite = {}
    table.insert(datTWrite, {
        Type = "type1",
        Key = "someKey",
        Value = data.SomethingInteresting
    })
    --and so on ad nauseum with an occasional but of actual logic to spice things up
    dataWriter:open(fileName .. ".bla")
    dataWriter:batchWrite(dataToWrite)
    dataWriter:close()
end

In this case, dataWriter is an instance of a predefined class that handles the process of writing to a file.

Answer 1

Without anything specific to go on, I would try researching code smells and see how your function compares. From the sounds of things, there's probably plenty for you to do. Are there blocks of similar / copied code isolated by different conditional logic? Nested loops or conditionals? These are some simple starting points when trying to split a large function into parts.

Answer 2

The good news is that you haven't stepped directly into the common Lua pessimization of concatenating strings to a buffer in a loop to build your output.

I would write your sample like this:

function buildTable(fileName, data)
    local t = {}
    t[#t+1] = {
        Type = "type1",
        Key = "someKey",
        Value = data.SomethingInteresting
    }
    --and so on ad nauseum with an occasional but of actual logic to spice things up
    dataWriter:open(fileName .. ".bla")
    dataWriter:batchWrite(t)
    dataWriter:close()
end

which has the small advantage of not using a long typo-prone name for the temporary table, and uses the t[#t+1] idiom to extend the array part which should be faster than calling table.insert().

Otherwise, the source of any structural improvements will be in the "and so on ad nauseum" part of the code.

• Look there for common calculations and fragments that can be collected into local functions.
• Remember that you can nest functions definitions, so helper functions can be constrained in scope to the place they are used.
• Look for too-clever logic, and rewrite them to be sensible to whoever will have to maintain it next year.
• wiki: Lua Design Patterns
• wiki: Zen Of Lua
• wiki: Optimisation Tips
• wiki: Profiling Lua Code

Above all, Beware the premature optimization. Benchmark what you have now as a point of comparison, and use that as a guide for finding performance bottlenecks.

Answer 3

By "and so on ad nauseum with an occasional but of actual logic to spice things up" I assume you mean you have many blocks like:

table.insert(datTWrite, {
    Type = "type1",
    Key = "someKey",
    Value = data.SomethingInteresting
})

The only aspect of this that is unique to the function is the table being filled and the data object. My personal "best practice" would be to pull all this out into a separate table like:

local entries = {
    {
        Type = "type1",
        Key = "someKey",
        ValueField = "SomethingInteresting",
    },
    {
        Type = "type2",
        Key = "someOtherKey",
        ValueField = "SomethingElse",
    },
    -- etc.
}

This table should either be a global or in a scope outside where the function is defined. Now you can more easily reconfigure the entries without making any changes to the function that does the actual work. The function itself is greatly simplified by iterating through entries thus:

for i, entry in ipairs(entries) do
    table.insert(datTWrite, {
        Type = entry.Type,
        Key = entry.Key,
        Value = data[entry.ValueField]
    })
end

For the "occasional" logic, each entry could have an optional function to give you interesting information in the loop. E.g.:

for i, entry in ipairs(entries) do
    if not entry.CheckSomething or entry.CheckSomething() then
        table.insert(datTWrite, {
            Type = entry.Type,
            Key = entry.Key,
            Value = data[entry.ValueField]
        })
    end
end

Alternatively, you could even allow individual entries in the table to BE functions if you need more customizability. Each entry function would return a table (or not).

for i, entry in ipairs(entries) do
    if type(entry) == "function" then
        local newEntry = entry()
        if newEntry then
            table.insert(datTWrite, newEntry)
        end
    else
        table.insert(datTWrite, {
            Type = entry.Type,
            Key = entry.Key,
            Value = data[entry.ValueField]
        })
    end
end