What can I do better?

[Brodur 0]

Asking honestly, what can I do better in this program? I am not overly familiar with lua and would love some pointers on how I could make this program better.

Also, pointers on getting event listening working would be great!

https://pastebin.com/rChVHKss     https://github.com/Brodur/Turret-Control

 

--- Turret Control

-- Helps manage OpenModularTurrets turret blocks that have serial IO

-- @Author: Brodur

-- @Version: 2.2

-- @Requires:

-- https://pastebin.com/WvF00A71 : JSON Utility functions

-- https://pastebin.com/pLpe4zPb : Menu functions

-- https://pastebin.com/SKwM2UZH : Lua JSON Library

 

local term = require("term")

local component = require("component")

local event = require("event")

local json = require("json")

local menu = require("menu")

local jsonutil = require("jsonutil")

local fs = require("filesystem")

 

local m = {}

local tb = "turret_base"

local lb = "-------------------------------"

 

local mmopts = {

"Summary",

"Targeting",

"User Admin",

"Push Config",

"Exit"

}

 

local db = {}

local toRemove = {}

local jsondir = "/home/settings.json"

 

--- Trims strings

-- Gets rid of trailing white space or special characters.

-- @param s The string to trim.

-- @return The trimmed string.

function trim(s)

return (s:gsub("^%s*(.-)%s*$", "%1"))

end

 

--- Summary output

-- Outputs the summary of a selected turret.

-- @param turrets A table containing the hardware addresses of turrets.

function m.summary()

term.setCursor(1,1)

print(mmopts[1] .. "\n")

print("Trusted players")

print(lb)

 

for k,v in pairs(db.users) do

print(" - " .. k, v and "[Admin]" or "[Trusted]")

end

print("\nAttacks")

print(lb)

print("Mobs:\t\t" .. tostring(db.targets.setAttacksMobs))

print("Neutrals:\t" .. tostring(db.targets.setAttacksNeutrals))

print("Players:\t" .. tostring(db.targets.setAttacksPlayers))

 

print("\n\nPress enter to continue...")

term.read()

end

 

--- Set Attack

-- Sets the attack setting for all turrets.

-- @param turrets A table containing the hardware addresses of turrets.

-- @param flag  1 or 2, corresponds to true or false, toggles the attack mode.

-- @param option  Which option was chosen in the target function

function m.setAttack(flag, option)

local methods = {"setAttacksMobs", "setAttacksNeutrals", "setAttacksPlayers"}

local arguments = {true, false}

db.targets[methods[option]] = arguments[flag]

db.hasChanges = true

end

 

---Target

-- Selects the targeting parameter

-- @param turrets A table containing the hardware addresses of turrets.

function m.target()

local options = {"Attack Mobs", "Attack Neutrals", "Attack Players", "Exit"}

local opt = -1

while opt ~= #options do

opt = menu.list(options, mmopts[2])

if opt ~= #options then

local flag = menu.dialog(options[opt], "True", "False")

m.setAttack(flag, opt)

else break end

end

end

 

--- Add Trusted Player

-- Adds a trusted player to all turrets, either with or without admin privileges.

-- @param player  The player to add.

-- @param usrType 1 for trusted, 2 for admin.

function m.addTrustedPlayer(player, usrType)

local args = {false, true}

for k,_ in pairs(db.users) do

if player == k then error("Cannot add a user that already exists!") end

end

db.users[player] = args[usrType]

db.hasChanges = true

end

 

--- Remove Trusted Player

-- Removes a user from the trusted players list.

-- @param player The user to remove.

function m.removeTrustedPlayer(player)

toRemove[#toRemove+1] = player

db.users[player] = nil

db.hasChanges = true

end

 

--- Users

-- Launches the add or remove user dialog

function m.users()

local options = {"Add a trusted user", "Remove a trused user","Exit"}

local opt = -1

while opt ~= #options do

opt = menu.list(options, mmopts[3])

if opt == 1 then

local userTypes = {"Trusted", "Admin"}

term.write("Add a trusted player: ")

local player = trim(term.read())

local usrType = menu.dialog("User or Admin?", "User", "Admin")

local opt = menu.dialog("Add \'" .. player .. "\' as " .. userTypes[usrType] .." type user?", "Confirm", "Cancel")

if opt == 1 then

m.addTrustedPlayer(player, usrType)

end

end

if opt == 2 then

local users = {"Cancel"}

for k,v in pairs(db.users) do users[#users+1] = k end

local user = -1

while user ~= 1 do

user = menu.list(users, "Select a user")

if user ~= 1 then

local player = users[user]

local confirm = menu.dialog("Remove \'" .. player .. "\' from trusted users?", "Confirm", "Cancel")

if confirm == 1 then

m.removeTrustedPlayer(player)

table.remove(users, user)

end

end

end

end

end

end

 

--- Distribute Json

-- Disseminates the settings from the database to all turrets.

function m.distribJson()

for _,v in pairs(db.turrets) do

for _,player in pairs(toRemove) do

component.invoke(v, "removeTrustedPlayer", player)

end

toRemove= {}

 

  for user,priv in pairs(db.users) do

component.invoke(v, "addTrustedPlayer", user, priv)

  end

for meth,bool in pairs(db.targets) do

component.invoke(v, meth, bool)

end

end

end

 

--- Sub menu

-- Determines which menu function to call.

-- @param index The selected index in the main menu options table.

function m.subMenu(index)

local turrets = {}

for k in pairs(component.list(tb)) do turrets[#turrets+1] = k end

db.turrets = turrets

 

if index == mmopts[1] then m.summary() end

if index == mmopts[2] then m.target() end

if index == mmopts[3] then m.users() end

if index == mmopts[4] then m.distribJson() end

end

 

--- On Load

-- Initializes the database and grabs the initial turrets.

function m.onLoad()

if not fs.exists(jsondir) then

db = {

    hasChanges=false,

    targets={setAttacksMobs=false,

         setAttacksNeutrals=false,

         setAttacksPlayers=false

        },

    turrets = {},

    users = {}

  }

  jsonutil.save(db, jsondir)

end

          

db = jsonutil.load(jsondir)

for k in pairs(component.list(tb)) do db.turrets[#db.turrets+1] = k end

m.main()

end

 

--- Main

-- Launches the main logic and displays the top level menu.

function m.main()

local mmoptTitle = "Main Menu"

local mmopt = -1

 

while mmopt ~= #mmopts do

if db.hasChanges then

jsonutil.save(db, jsondir)

db.hasChanges = false

m.distribJson()

end

if mmopt ~= #mmopts then

mmopt = menu.list(mmopts, mmoptTitle)

m.subMenu(mmopts[mmopt])

end

end

end

 

--- On Component Added

-- Push the stored config when a new turret is detected.

-- @param eventType The type of event, passed by event listener.

-- @param address The hardware address of the new component.

-- @param componentType   The type of the new component.

-- @todo figure out why the event listener is not firing all of the time, until that is figured disregard this function

-- function onComponentAdded(eventType, address, componentType)

-- if componentType == "turret_base" then

-- distribJson()

-- end

-- end

 

-- event.listen("component_added", onComponentAdded)

m.onLoad()

return m

 

[Molinko 43]

Overall the program looks pretty solid. Personally I like to write things with more descriptive names for when I have to revisit programs after a while. One place where things become unclear is using 'k' or 'v' in your pairs loops. If the keys and values are really arbitrary then this makes sense however several places in your code could be clarified by using better variable names. Here's an example..

--- Distribute Json
-- Disseminates the settings from the database to all turrets.
function m.distribJson()
  -- # also db.turrets is an array so maybe ipairs would help a reader infer that. _ lets me see the index/key is irrelevant in this loop but perhaps some loop abstraction would be nice?
  for _,v in pairs(db.turrets) do -- # what exactly is v? maybe this chould be compId or turret_id?
    for _,player in pairs(toRemove) do
      component.invoke(v, "removeTrustedPlayer", player) -- # seems v is a uuid(component address)?
    end
 
    for user,priv in pairs(db.users) do -- # like this! this is more clear
      component.invoke(v, "addTrustedPlayer", user, priv)
    end
    for meth,bool in pairs(db.targets) do -- # bool could be more clear. perhaps should_engage or something that describes the variable purpose
      component.invoke(v, meth, bool)
    end
  end
  toRemove= {}
end

local privs = {
  [1] = true, -- # equal to admin
  [2] = false, -- # generic trusted priv
  admin = true, -- # named privs are nice to have
  trusted = false --# maybe 'user' is too generic? perhaps 'basic' or 'trusted'?
}

function m.addTrustedUser(user, level)
  if db.users[user] then error "Cannot add an existing user!" end
  db.users[user] = level
  db.hasChanges = true
end

m.addTrustedUser('molinko', privs[2]) -- # or privs.trusted this is nice and readable.
m.addTrustedUser('Brodur', privs.admin) -- #  I dont have to look at the privs table to see whats happening...

Overall your program looks fine. Personally I would just clarify(imply) whats happening a bit more with your variable names and change some to do the same.

Nice program though

[Molinko 43]

Yeah it would.. I missed that. Perhaps a nil check would be better.

function m.addTrustedUser(user, level)
  if db.users[user] ~= nil then error "Cannot add an existing user!" end
  db.users[user] = level
  db.hasChanges = true
end

 

[Brodur 0]

1 hour ago, Molinko said:

if db.users[user] then error "Cannot add an existing user!" end

I agree with all of your points, thanks! 

One question, in the above quote would this not be undesirable? As I store my users in a table where they are the key and a boolean is the value. So if the user was false, aka trusted but not admin, would the above code not function properly?

 

{

"hasChanges": false,

"users": {

"Brodur": true,

"crassclown": true,

"fatso12321": true,

"Ouhai_Ruby": true,

"Kyle8910": false,

"Sacreddeathflame": false,

"Emfitty": false

},

"targets": {

  "setAttacksMobs": false,

"setAttacksNeutrals": false,

  "setAttacksPlayers": false

},

"turrets": [

]

}

[Brodur 0]

Right on, thanks.