[Enhancement] Avoid global namespace pollution
Road-block opened this issue ยท 7 comments
The saved variables names selected are too generic.
MiniMapDB
KeyboardPosition
MousePosition
ShowEmptyBinds
ShowInterfaceBinds
KeyBindSettings
KeyBindSettingsMouse
MouseKeyEditLayouts
KeyboardEditLayouts
CurrentLayoutMouse
CurrentLayoutKeyboard
tutorialCompleted
These are all put in the global namespace after ADDON_LOADED fires for KeyUI.
That means they can overwrite or get overwritten by other addons leading to really hard to debug issues.
I'm not mentioning the KeyUI_Settings
variable because that one is actually properly named <addonname>_suffix
is a good naming scheme because the addonname is always unique for a specific player's installation (you cannot have 2 folders with the same name in \AddOns).
The solution to this problem can be one of
- Prefix the existing variables with
KeyUI_
- Tuck them inside the
KeyUI_Settings
variable (I would go with this)
The second one will also make the variables migration quite easy.
So they would be
KeyUI_Settings.MinimapDB
KeyUI_Settings.KeyboardPosition
..
etc.
โThank you for your advice! This is my first coding project, so Iโm still learning as I go. I really appreciate your feedback and am open to any suggestions you may have!
It's a well made addon, especially for a first project ๐
Code is fairly clean and for the most part well encapsulated, you're using the ... vararg passed in by the client to namespace your functions etc.
So lots of things already done well.
The saved variables thing catches a lot of people out in their first projects, because it's natural to assume they are something unique to your addon.
Unfortunately they are not they are just Lua arrays / tables no different to any other array / table defined in code.
You can do /dump MiniMapDB
in-game and it will print that array contents to chat.
You can do /run MiniMapDB={}
and it will reset it.
Other addons can accidentally do that if they are leaking a MiniMapDB variable or using it as a name for their own minimap settings.
In the latter case, if two addons are using the same name to reference their minimap settings, the one that loads last (alphabetic order) will overwrite the first one, or more likely try to use the other addon's settings
So like you do in your actual code trying to encapsulate variables and functions and keep them local to your addon the same applies to saved variable table names.
Since you cannot avoid putting them in the global namespace that all addons share, the best you can do is try give them a name that is likely to be unique (<addonname>_suffix
is a good naming convention for saved variables.
I'll try send a pull request so you can see it in practice.
Edit: I want to say this is nothing urgent, the addon works fine as is. It's more about future-proofing than anything.
Something I would like to point out if it helps.
You are already using this facility but not fully ๐บ
The wow client passes into every Lua file referenced in the .toc of an addon a vararg ...
First argument is the addon folder name (string) and second argument is a Lua table (array) that is shared between all the addon files without being global.
It is a - particular to WoW - "my addon scope".
local addonName, addon = ...
on top of your Lua files lets you pass around variables (including methods, other tables etc) without having to put them in the global namespace that all addons share.
So let's say you have an inCombat
boolean that you want to use in two of your addon files but does not need to persist in saved variables.
addon.inCombat = true
in file1.lua is perfectly accessible in file2.lua so long as it also has gotten a reference to local addonName, addon = ...
at the top.
Iโm grateful for your input and have now implemented almost everything. There are still a few variables Iโm not fully satisfied with, but Iโm working on them.
Would you be interested in taking a quick look?