Is this smart programming (Understading lua).

[Yukyuk]

Working on a new addon and I want to use constants.
My constans are placed in a seperate lua file that gets loaded before the rest of the lua files.

My question is about the Color constants (end of the code).
I want to use them as follows:

addon:Debug(private.COLORS["GREEN"].hex .. "variable text" .. private.COLORS["END"].hex)

Is this a good way to do this?
Is it better to "copy" (local COLORS = private.COLORS) to other lua files before using the COLORS table?
Is there another (easier/better) way I have't thought of?

Any advice about this topic by more experienced lua users is appriciated.

--[[    *** Deathcounter ***
Written by : Yukyuk, EU-Moonglade
--]]
 
-------------------------------------------------------------------------------
--  From _Global to local.
-------------------------------------------------------------------------------
local _G = _G
 
local tinsert   = _G.table.insert
local tremove   = _G.table.remove 
local tsort     = _G.table.sort 
local twipe     = _G.table.wipe
 
local pairs     = _G.pairs
local ipairs    = _G.ipairs
 
 
-------------------------------------------------------------------------------
--  AddOn namespace.
-------------------------------------------------------------------------------
local FOLDER_NAME, private = ...
 
 
-------------------------------------------------------------------------------
--  AddOn constants.
-------------------------------------------------------------------------------
private.addon_name = "Deathcounter"
private.addon_version = "1.0"   
 
 
-------------------------------------------------------------------------------
--  Player constants.
-------------------------------------------------------------------------------
private.PLAYER_NAME     = _G.UnitName("player")
private.PLAYER_FACTION  = _G.UnitFactionGroup("player")
private.REALM_NAME      = _G.GetRealmName() 
 
 
-------------------------------------------------------------------------------
--  Color constants
-------------------------------------------------------------------------------
private.COLORS = {
    ["GREEN"]   = {hex = "|cff00ff00"},
    ["RED"]     = {hex = "|cffff0000"},
    ["END"]     = {hex = "|r"}
    }

[lightspark]

Originally Posted by Yukyuk Is this a good way to do this?

It's a good way to keep your code tidy. As for you colours table, if you only keep hex values, why dun you just store it this way?

private.COLORS = {
    ["GREEN"]   = "|cff00ff00",
    ["RED"]     = "|cffff0000",
    ["END"]     = "|r",
}

Having one extra table isn't cool...

Originally Posted by Yukyuk Is it better to "copy" (local COLORS = private.COLORS) to other lua files before using the COLORS table?

You can read Phanx' post on this topic. I prefer to create shortcuts to certain tables, but private.COLORS is fine.

Originally Posted by Yukyuk Is there another (easier/better) way I have't thought of?

Not really...

[Phanx]

Originally Posted by Yukyuk Is it better to "copy" (local COLORS = private.COLORS) to other lua files before using the COLORS table?

It depends on how often you're going to access the table. If you're only reading it occasionally to print a message to the user, you don't need an upvalue. If you're looking up colors every time a combat log event occurs, inside an OnUpdate script, or even in response to something like UNIT_HEALTH, you should use an upvalue.

On a side note, "local colors = private.COLORS" is not creating a copy of the table; it's only creating a "shortcut" to accessing the same table. This shortcut is called an "upvalue" (shortcut to a value up in a higher scope) or a "local reference". It's like using a bookmark to quickly find your place the next time you open the book, instead of having to flip through all the pages and figure out where you were, or scan through the table of contents. However, you don't want to upvalue everything -- if you end up with a bookmark on every page that's just as bad, so you need to have a balance.

Originally Posted by Yukyuk Is there another (easier/better) way I have't thought of?

Again, it really depends on what you're doing, but I would just write "|r" directly in your string and leave it out of your table. It's entirely static (eg. you could later decide you wanted a lighter red and change the hex code for "RED" accordingly, but there's nothing to change with "|r") and typing "|r" is much easier/faster/shorter (and more readable) than typing "private.COLORS["END"]".

Personally, I wouldn't even use a table at all for just two colors. I'd just create two variables:

private.COLOR_GREEN = "|cff00ff00"
private.COLOR_RED = "|cffff0000"

...and whether I'd upvalue those in other files would depend on how often they were being accessed. I would probably not bother with a table in this scenario until I was up to 5+ colors.

However, if you later made the colors user-editable with an in-game GUI (so they were stored in saved variables) then I would put them in a table even with just 2, to keep them separated from other settings, eg.

MyAddonSettings = {
     blah = 57,
     fdsfdafda = true,
     colors = {
        red = "|cffff0000",
        green = "|cff00ff00",
     }
}

-------------------------------------------

Finally, you should reconsider whether putting all your constants in a separate file is actually beneficial. Loading files is by far the slowest part of the addon loading process, so unless you have 5000 constants (and if you do, then you should probably reconsider your whole addon design!) it's probably better just to put them at the top of your main file, or (if your addon has multiple files) just define them as locals in the file that actually uses them.

[MunkDev]

local tinsert   = table.insert
local tremove   = table.remove 
local tsort     = table.sort 
local twipe     = table.wipe
 
local pairs     = pairs
local ipairs    = ipairs
 
-------------------------------------------------------------------------------
--  AddOn namespace.
-------------------------------------------------------------------------------
local FOLDER_NAME, private = ...
 
-------------------------------------------------------------------------------
--  AddOn constants.
-------------------------------------------------------------------------------
private.addon_name = "Deathcounter" -- Isn't this FOLDER_NAME?
private.addon_version = "1.0" -- This could be placed in the .toc file
 
-------------------------------------------------------------------------------
--  Player constants.
-------------------------------------------------------------------------------
private.PLAYER_NAME     = UnitName("player")
private.PLAYER_FACTION  = UnitFactionGroup("player")
private.REALM_NAME      = GetRealmName()    
 
-------------------------------------------------------------------------------
--  Color constants
-------------------------------------------------------------------------------
private.COLORS = {
    GREEN   = "|cff00ff00%s|r",
    RED     = "|cffff0000%s|r",
}

Upvalueing the global table is pretty pointless if you're not doing thousands of lookups. The same can be said about storing name, faction and realm. Also, my personal preference for color escape sequences is to use %s and format the string instead of concatenating. Depending on what you're using it for, this might be a better approach:

print(format(private.COLORS.RED, "This text is red"))
print(format(private.COLORS.GREEN, "This text is green"))

> This text is red > This text is green

[Phanx]

Originally Posted by MunkDev Also, my personal preference for color escape sequences is to use %s and format the string instead of concatenating.

Calling a function is more expensive than a simple concatenation. There's no excuse for using string.format just to join two strings together.

[MunkDev]

Originally Posted by Phanx Calling a function is more expensive than a simple concatenation. There's no excuse for using string.format just to join two strings together.

Hence, depends on usage. I just use that approach to keep the code readable and it's not exactly a huge overhead to format a string every now and then. I personally don't use format for simple concatenation, but I do use it in cases where I frequently use differing strings of the same format. Like this:

OVERRIDE            = "%s is already bound to %s.\nPress |T%s:20:20:0:0|t again to continue anyway.",
SUCCESS             = "|T%s:16:16:0:0|t was successfully bound to %s.",
CONTINUE            = "Press |T%s:20:20:0:0|t again to continue.",
CONFIRM             = "Press |T%s:20:20:0:0|t again to confirm.",

[Lombra]

I do like format for readability. At least if there's more than one variable, or if it's not at the beginning or the end.

[Phanx]

Yes, string.format is fine and good for localization or inserting many variables into long strings, but that's not what we're talking about. You suggested doing this:

local msg = string.format("%s%s", private.COLOR_RED, "This text is red")

... instead of this:

local msg = private.COLOR_RED .. "This text is red"

... and there's really no excuse for that, even if you're only doing it once. If you just want to prepend a static value to a string, just do that. Don't overcomplicate things. This isn't even about program optimization; it's about just using the most straightforward method to achieve your goal. Using string.format in this case is as silly as using select(1, ...) -- which I have sadly seen, more than once, in live code.

[MunkDev]

Sure, formatting is redundant in this case. But I actually suggested...

local msg = format(private.COLORS.GREEN, "variable text")

...instead of...

local msg = private.COLORS["GREEN"].hex .. "variable text" .. private.COLORS["END"].hex

...for the sake of readability. Your intentionally complicated extension of that is not very readable.

[Yukyuk]

Thank you all for the responses.
Learned (again) from this discussion.

For me personally this piece was the most valuable.

On a side note, "local colors = private.COLORS" is not creating a copy of the table; it's only creating a "shortcut" to accessing the same table. This shortcut is called an "upvalue" (shortcut to a value up in a higher scope) or a "local reference". It's like using a bookmark to quickly find your place the next time you open the book, instead of having to flip through all the pages and figure out where you were, or scan through the table of contents. However, you don't want to upvalue everything -- if you end up with a bookmark on every page that's just as bad, so you need to have a balance.

Keeping my code tidy matters to me quite a bit.

It's a good way to keep your code tidy

Have been programming most of my adult life (in various languages) and keeping my code clean, readable and maintainable matters.

Thats why I want to have a beter understanding of lua, because my lua coding skills still need a lot of work.

[kurapica.igas]

I suggest in some post to use environment control to make your code more simple to be read.

If global.lua is your first lua file in toc.

-- Modify the environment, using private table as environment
setfenv(1, setmetatable( select(2, ...), { __index = function(self, key)
    local val = _G[key]
    if val ~= nil then rawset(self, key, val) end
    return val
end } ))
 
-- Global part
-------------------------------------------------------------------------------
--  AddOn constants.
-------------------------------------------------------------------------------
addon_name = "Deathcounter" -- Isn't this FOLDER_NAME?
addon_version = "1.0" -- This could be placed in the .toc file
 
-------------------------------------------------------------------------------
--  Player constants.
-------------------------------------------------------------------------------
PLAYER_NAME     = UnitName("player")
PLAYER_FACTION  = UnitFactionGroup("player")
REALM_NAME      = GetRealmName()    
 
-------------------------------------------------------------------------------
--  Color constants
-------------------------------------------------------------------------------
COLORS = {
    GREEN   = "|cff00ff00%s|r",
    RED     = "|cffff0000%s|r",
}

When use in your addons' other file :

setfenv(1, select(2, ...) )
 
addon:Debug(COLORS["GREEN"].hex .. "variable text" .. COLORS["END"].hex)

Use a private environment can make your codes be shared in every lua files and without taint the _G.

About the optimization, maybe you can check this post http://www.wowinterface.com/forums/s...d.php?p=283565

[zork]

Interesting approach. But is there something wrong about doing this?

A.lua

local addonName, addonTable = ...
 
  addonTable.G = {} --global
  addonTable.L = {} --local (addon only)
  addonTable.C = {} --config
  addonTable.DB = {} --database
 
  -------------------------------------
  -- VARIABLES
  -------------------------------------
 
  addonTable.L.constants = {
    name          = addonName,
    version       = GetAddOnMetadata(L.name, "Version"),
    locale        = GetLocale()  
  }
  
  --make table globally available
  MyAddonGlobalName = addonTable.G

B.lua

local an, at = ...
local L,C = at.L, at.C --just get what you need
 
--create button func
function L:CreateButton(parent,name,text,adjustWidth,adjustHeight)
 
  --button frame
  local b = CreateFrame("Button", name, parent, "UIPanelButtonTemplate")
  b.text = _G[b:GetName().."Text"]
  b.text:SetText(text)
  b:SetWidth(b.text:GetStringWidth()+(adjustWidth or 20))
  b:SetHeight(b.text:GetStringHeight()+(adjustHeight or 12))
 
  return b
 
end

If I expose addonTable.G to global scope. Can another addon iterate it and access addonTable.L?

[Fizzlemizz]

addonTable is private to the addon so unless there is a way to iterate up (a GetParent() for table structures) only the .G branch is available outside the addon via MyAddonGlobalName.

[Phanx]

Originally Posted by kurapica.igas I suggest in some post to use environment control to make your code more simple to be read.

It's an absurd overreaction to create a custom environment instead of just typing the keyword "local" in the appropriate places. That doesn't make your code more readable -- it just lets you be lazy and not have to understand or pay attention to proper variable scoping.

[Phanx]

Originally Posted by zork Interesting approach. But is there something wrong about doing this? local addonName, addonTable = ...     addonTable.G = {} --global   addonTable.L = {} --local (addon only)   addonTable.C = {} --config   addonTable.DB = {} --database If I expose addonTable.G to global scope. Can another addon iterate it and access addonTable.L?

1. There's nothing wrong with that, but it's not "solving" the same "problem" -- kurapica.igas was proposing to use a custom function environment to avoid the use of "local" keywords.

2. There's no "get parent" method for tables. so if you're in t.G you cannot access t, or any other keys/subtables in t unless there's a pointer to it in t.G.

3. Names like "G" and "C" suck. If you come back to your code after 6 months or a year, are you going to remember what those even mean? Addons aren't macros, space is not limited, and there's no good reason to not use meaningful variable/key names. If you need a comment to remember what your variable is for, you're doing something wrong. It should be immediately obvious anywhere in your code what the purpose of a variable is -- not just at the point where it's initially defined.

[kurapica.igas]

Originally Posted by Phanx It's an absurd overreaction to create a custom environment instead of just typing the keyword "local" in the appropriate places. That doesn't make your code more readable -- it just lets you be lazy and not have to understand or pay attention to proper variable scoping.

Control environment is very useful in many ways, sharing data, avoid taint and control variable scope, if you mean two file may create same name variable, nested environment can be used like :

Module "MyAddon"

Module "MyAddon.SomeModule"

It won't block you using 'local' on frequently called apis to gain some performance improvement.

To make an add-on with many files, using nested environments to separate them is an easy way to control the data sharing.

The main problem of using environment is it'd cost time for learning and testing, it's not required for beginner. So I only suggest it, not recommend it.

[SDPhantom]

Originally Posted by kurapica.igas Originally Posted by Phanx Originally Posted by kurapica.igas I suggest in some post to use environment control to make your code more simple to be read. It's an absurd overreaction to create a custom environment instead of just typing the keyword "local" in the appropriate places. That doesn't make your code more readable -- it just lets you be lazy and not have to understand or pay attention to proper variable scoping. Control environment is very useful in many ways, sharing data, avoid taint and control variable scope The main problem of using environment is it'd cost time for learning and testing, it's not required for beginner. So I only suggest it, not recommend it.

There only proper use for isolated environments is to restrict what untrusted code can access and/or execute. It's a security device, not a crutch for poor variable scoping.

[kurapica.igas]

Originally Posted by SDPhantom There only proper use for isolated environments is to restrict what untrusted code can access and/or execute. It's a security device, not a crutch for poor variable scoping.

If you mean the restricted environment used by secure template, that's yes, blz only use the environment control on that feature.

For lua, it's just a common feature.

In small add-on, with little features, control the variable scope seems meaningless, one table could to contains it all, the result would be same no matter you using environment control or not.

For an add-on with many of files, files are separated by their features, control the file's variable scoping will be useful to reduce the bugs.

And there are more in it, by control the environments, it would be able to provide more features, take an example, register event and handle them directly in the files like :

Module "xxxx"
 
function OnLoad(self)
    self:RegisterEvent("UNIT_AURA")
end
 
function UNIT_AURA(self, ...)
   -- ...
end

All those can be done without setfenv, that's right. So just the code for lazy guys.

[SDPhantom]

Originally Posted by kurapica.igas If you mean the restricted environment used by secure template, that's yes, blz only use the environment control on that feature.

I mean that's all it ever should be used for. Blizzard uses it for secure handlers, others use it to run a nested Lua interpreter inside the main Lua engine that runs code written by an end-user. I've even seen examples of it used to implement a Lua-based OS using isolated environments to run programs that happen to be other Lua files.

These are legitimate needs for isolated environments. Your method is just a suggested replacement for variable scoping and in the long run, largely hinders performance. Not to mention adding another layer of unnecessary complexity to the code in addition to denying access to every crucial Lua and WoW API if the addon author wasn't wise enough to include references to said functions in each new environment.

[Phanx]

Originally Posted by kurapica.igas For an add-on with many of files, files are separated by their features, control the file's variable scoping will be useful to reduce the bugs.

If someone is running into bugs caused by improper variable scoping that frequently, they need to learn how variable scoping works, not how to employ a(n expensive) workaround to shield them from their own incompetence and/or inexperience. And besides, if someone is not yet advanced enough as a programmer to understand how variable scoping works, they're almost certainly not yet advanced enough to understand how custom environments work either.

(Also, this isn't even the first time you've come into a thread that is clearly asking for advice/tips for new [to WoW] programmers, and started suggesting this overcomplicated setfenv nonsense. )