8
\$\begingroup\$

Creating New Scripts Dynamically in Lua

For absolute clarity, here is the code that the questions asks to review (this is ):

local ScriptWriter = {}

local twoSpaceIndent = "  "
local equalsWithSpaces = " = "
local newLine = "\n"

--scriptNameToCreate must be a string
--argumentsForNew and instanceVariablesToCreate must be tables and not nil
function ScriptWriter:new(scriptNameToCreate, argumentsForNew, instanceVariablesToCreate)

    local instance = setmetatable({}, { __index = self })

    instance.name = scriptNameToCreate
    instance.newArguments = argumentsForNew
    instance.instanceVariables = instanceVariablesToCreate
    instance.stringList = {}

    return instance
end
function ScriptWriter:makeFileForLoadedSettings()
    self:buildInstanceMetatable()
    self:buildInstanceCreationMethod()
    self:buildSettersAndGetters()
    self:buildReturn()
    self:writeStringsToFile()
end

--very first line of any script that will have instances
function ScriptWriter:buildInstanceMetatable()
    table.insert(self.stringList, "local " .. self.name .. " = {}" .. newLine)
    table.insert(self.stringList, newLine)
end

--every script made this way needs a new method to create its instances
function ScriptWriter:buildInstanceCreationMethod()
    --new() function declaration
    table.insert(self.stringList, ("function " .. self.name .. ":new("))
    self:buildNewArguments()
    table.insert(self.stringList, ")" .. newLine)

    --first line inside :new() function
    table.insert(self.stringList, twoSpaceIndent .. "local instance = setmetatable({}, { __index = self })" .. newLine)

    --add designated arguments inside :new()
    self:buildNewArgumentVariables()

    --create the instance variables with the loaded values
    for key,value in pairs(self.instanceVariables) do
      table.insert(self.stringList, twoSpaceIndent .. "instance." .. key .. equalsWithSpaces .. value .. newLine)
    end

    --close the :new() function
    table.insert(self.stringList, twoSpaceIndent .. "return instance" .. newLine)
    table.insert(self.stringList, "end" .. newLine)
    table.insert(self.stringList, newLine)
end
function ScriptWriter:buildNewArguments()
     --if there are arguments for :new(), add them
    for key,value in ipairs(self.newArguments) do
      table.insert(self.stringList, value)
      table.insert(self.stringList, ", ") 
    end
    if next(self.newArguments) ~= nil then --makes sure the table is not empty first
      table.remove(self.stringList) --remove the very last element, which will be the extra ", "
    end
end
function ScriptWriter:buildNewArgumentVariables()
    --add the designated arguments to :new()
    for key, value in ipairs(self.newArguments) do
      table.insert(self.stringList, twoSpaceIndent .. "instance." .. value .. equalsWithSpaces .. value .. newLine)
    end
end

--the instance variables need separate code because their names have to be the key and not the argument name
function ScriptWriter:buildSettersAndGetters()
    for key,value in ipairs(self.newArguments) do
        self:buildArgumentSetter(value)
        self:buildArgumentGetter(value)
        table.insert(self.stringList, newLine)
    end
    for key,value in pairs(self.instanceVariables) do
        self:buildInstanceVariableSetter(key, value)
        self:buildInstanceVariableGetter(key, value)
        table.insert(self.stringList, newLine)
    end
end
--code for arguments passed in
function ScriptWriter:buildArgumentSetter(variable)
    table.insert(self.stringList, "function " .. self.name .. ":set_" .. variable .. "(newValue)" .. newLine)
    table.insert(self.stringList, twoSpaceIndent .. "self." .. variable .. equalsWithSpaces ..  "newValue" .. newLine)
    table.insert(self.stringList, "end" .. newLine)
end
function ScriptWriter:buildArgumentGetter(variable)
    table.insert(self.stringList, "function " .. self.name .. ":get_" .. variable .. "()" .. newLine)
    table.insert(self.stringList, twoSpaceIndent .. "return " .. "self." .. variable .. newLine)
    table.insert(self.stringList, "end" .. newLine)
end
--code for instance variable values passed in
function ScriptWriter:buildInstanceVariableSetter(key, variable)
    table.insert(self.stringList, "function " .. self.name .. ":set_" .. key .. "(newValue)" .. newLine)
    table.insert(self.stringList, twoSpaceIndent .. "self." .. key .. equalsWithSpaces .. "newValue" .. newLine)
    table.insert(self.stringList, "end" .. newLine)
end
function ScriptWriter:buildInstanceVariableGetter(key, variable)
    table.insert(self.stringList, "function " .. self.name .. ":get_" .. key .. "()" .. newLine)
    table.insert(self.stringList, twoSpaceIndent .. "return " .. "self." .. key .. newLine)
    table.insert(self.stringList, "end" .. newLine)
end

--last line of any script that will have instances
function ScriptWriter:buildReturn()
    table.insert(self.stringList, "return " .. self.name)
end

function ScriptWriter:writeStringsToFile()
    local fileName = (self.name .. ".lua")
    file = io.open(fileName, 'w')
    for key,value in ipairs(self.stringList) do
      file:write(value)
    end
    file:close()
end

return ScriptWriter

The other two code snippets provided in the question are provided purely as context code.

Moreover, the last bit of the questions makes it clear what the asker wants:

I am not sure if I am correct that this could be useful in certain situations. What I am looking for is feedback on the readability of the code, and following Lua best practices. I would also love to hear whether this approach is a valid one, and whether the way that I have done things will be extensible.

This question sounds on-topic to me. And I can't see what in the provided code snippet would warrant moving the question.

Did this question get moved because of the context examples provided? That seems highly inappropriate. Would this question be better for CodeReview without the usage and output examples?


The actual reason this question was migrated seems to be because the migrator determined this question was off-topic for Code Review, but was still interesting and would work on another SE website. I understand that this question could be a good fit for where it was migrated to, so if we could instead focus on whether or not it's a good fit for Code Review and if not, why, that would be appreciated.

\$\endgroup\$
  • 1
    \$\begingroup\$ For what it's worth, when I answered this question (while it was still on CR) it felt like I was giving more of a Programmers answer than a CR answer. I was not at all surprised to see it migrated. \$\endgroup\$ – Dagg Sep 1 '14 at 1:18
6
\$\begingroup\$

I gave my rationale in a comment when I migrated the question. Identifiers such as ScriptIAmMaking and test are placeholders akin to foo and bar, making it example code and therefore off-topic for Code Review. Are those identifiers peripheral to the question? I didn't make that distinction at the time, as the question didn't obviously state what code was in scope or out of scope for review.

The author also expressed extreme doubt about the approach, with statements like:

I am not entirely sure of what the use cases would be just yet.

At this point, this is all theoretical, though.

I would also love to hear whether this approach is a valid one

which indicate that there is a design question in addition to the code review question. Since the question of whether the code is appropriate at all would logically precede the decision to review the code, I migrated it to Programmers.

In retrospect, I think that asking the author to post a separate question on Programmers might have been a better approach. If @bazola still wants a review of ScriptWriter, I invite him to post it again on Code Review, hopefully with conviction and a realistic use case.

\$\endgroup\$
  • \$\begingroup\$ But "super secret website url" doesn't count as placeholder akin to foo or bar? \$\endgroup\$ – nhgrif Aug 23 '14 at 17:35
  • \$\begingroup\$ @nhgrif As will all matters, there is a judgement call to be made. A possible distinguishing criterion is that ScriptIAmMaking is an identifier (code), and "super secret website url" is a string (data). But don't quote that as a precedent. \$\endgroup\$ – 200_success Aug 23 '14 at 17:45
  • 1
    \$\begingroup\$ But ScriptIAmMaking only appears a) as an argument (data--as you point out about "super secret website url") in the example of calling the script maker, and b) in the output example. I agree that these sections could have been more clearly marked as contextual code only, but had they been, using "ScriptIAmMaking" seems it should have been perfectly fine. In fact, I think something that seems so clearly a placeholder is good for the example code. It's what helped me identify these sections as example rather than the actual code intending to be reviewed. \$\endgroup\$ – nhgrif Aug 23 '14 at 17:49
  • \$\begingroup\$ @nhgrif Fair points. \$\endgroup\$ – 200_success Aug 23 '14 at 17:54
4
\$\begingroup\$

Reading the question I get the feeling that this question is split in two parts:

  1. Review my code, please.
  2. Review my approach (from viewpoint of all programmers), and whether it makes sense.

This question seems to be well-accepted on programmers, but IMO didn't need to be migrated.

Instead the correct approach would have been to comment to OP that reviewing approaches in specifics does not work so well for CR. The part of the question inquiring:

I would also love to hear whether this approach is a valid one, and whether the way that I have done things will be extensible.

Should have been removed, and the question would have been a perfect fit for our scope.

I thus conclude: The decision was not conform with standard migration regulations on SX-network ("Don't migrate on-topic questions"), but in its current form somewhat understandable, as the question was also well off on Programmers.

I would have possibly favored editing and a repost on Programmers over migration, though.

\$\endgroup\$
  • \$\begingroup\$ While asking the question you have in block quotes makes it also a good fit for Programmers, the comments on the question suggested that it was moved because asking about the parts that make it a good fit was premature since the question contains placeholders (which are nowhere to be found) and the comment claims the question to be off-topic, which I just don't see in the slightest. \$\endgroup\$ – nhgrif Aug 23 '14 at 13:59
  • \$\begingroup\$ @nhgrif I am fully with you there. If that doesn't come off clear, feel free to edit. \$\endgroup\$ – Vogel612 Aug 23 '14 at 14:05

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .