Quantcast How could this be written neater? - WoWInterface
Thread Tools Display Modes
05-07-17, 09:41 AM   #1
Layback_
An Onyxian Warder
Join Date: Feb 2016
Posts: 356
How could this be written neater?

Hi all,

I've tried to make an aura tracker addon and..... it is working fine, but the code looks really disgusting.

Lua Code:
  1. -- Upvalue variables
  2. local _G = getfenv(0);
  3.  
  4. local GetSpecialization = _G.GetSpecialization;
  5. local GetSpellInfo = _G.GetSpellInfo;
  6. local ReloadUI = _G.ReloadUI;
  7.  
  8. -- Local variables
  9. local size = 40;
  10. local point = {"CENTER", UIParent, "CENTER", 0, -110};
  11. local gap = 2;
  12. local max = 6;
  13.  
  14. local buffIDList = {
  15.     ["WARLOCK"] = {
  16.         [3] = { -- Destruction
  17.             235156,
  18.             215165,
  19.             119899,
  20.         },
  21.     },
  22. };
  23.  
  24. -- Local functions
  25. local InitObjects;
  26.  
  27. -- AuraTrack
  28. local AuraTrack = CreateFrame("Frame");
  29. AuraTrack:RegisterEvent("ADDON_LOADED");
  30. AuraTrack:RegisterEvent("PLAYER_ENTERING_WORLD");
  31. AuraTrack:SetScript("OnEvent", function(self, event, ...)
  32.     self[event](self, ...);
  33. end);
  34.  
  35. function AuraTrack:ADDON_LOADED(...)
  36.     if ... == "MyAuraTracker" then
  37.         InitObjects();
  38.  
  39.         self:UnregisterEvent("ADDON_LOADED");
  40.     end
  41. end
  42.  
  43. function AuraTrack:PLAYER_ENTERING_WORLD()
  44.     local holder = self.holder;
  45.  
  46.     holder:RegisterUnitEvent("UNIT_AURA", "player");
  47.     holder:SetScript("OnEvent", function(self, event, ...)
  48.         AuraTrack[event](AuraTrack, ...);
  49.     end);
  50.  
  51.     self:UnregisterEvent("PLAYER_ENTERING_WORLD");
  52. end
  53.  
  54. function AuraTrack:UNIT_AURA(...)
  55.     local holder = self.holder;
  56.  
  57.     -- Get player's class and spec to access buffIDList table
  58.     local _, class = UnitClass(...);
  59.     local spec = GetSpecialization();
  60.  
  61.     -- For each button, check if they are already assigned with aura
  62.     for i = 1, #holder.button do
  63.         local button = holder.button[i];
  64.  
  65.         -- If the button is assigned with aura and that aura is on-going, update its duration and so on
  66.         -- Otherwise, remove id from button and hide
  67.         if button.spellID then
  68.             local spellName = GetSpellInfo(button.spellID);
  69.  
  70.             local _, _, icon, count, _, duration, expires = UnitAura(..., spellName);
  71.  
  72.             if duration and duration > 0 then
  73.                 button.cd:SetCooldown(expires - duration, duration);
  74.  
  75.                 button.icon:SetTexture(icon);
  76.  
  77.                 button.count:SetText((count and count > 0) and count or "");
  78.  
  79.                 button:Show();
  80.             else
  81.                 button.spellID = nil;
  82.  
  83.                 button:Hide();
  84.             end
  85.         end
  86.     end
  87.  
  88.     -- For each buff, check if they are activated
  89.     for i = 1, #buffIDList[class][spec] do
  90.         local spellName = GetSpellInfo(buffIDList[class][spec][i]);
  91.  
  92.         local _, _, icon, count, _, duration, expires = UnitAura(..., spellName);
  93.  
  94.         -- If the buff is activated, see if that buff is already assigned to a button
  95.         if duration and duration > 0 then
  96.             local assigned = false;
  97.  
  98.             for j = 1, #holder.button do
  99.                 if holder.button[j].spellID == buffIDList[class][spec][i] then
  100.                     assigned = true;
  101.                 end
  102.             end
  103.  
  104.             -- If buff is not assigned to any of buttons,
  105.             -- assign the id and update its duration and so on
  106.             if not assigned then
  107.                 for j = 1, #holder.button do
  108.                     local button = holder.button[j];
  109.  
  110.                     if not button.spellID then
  111.                         button.spellID = buffIDList[class][spec][i];
  112.  
  113.                         button.cd:SetCooldown(expires - duration, duration);
  114.  
  115.                         button.icon:SetTexture(icon);
  116.  
  117.                         button.count:SetText((count and count > 0) and count or "");
  118.  
  119.                         button:Show();
  120.                        
  121.                         break;
  122.                     end
  123.                 end
  124.             end
  125.         end
  126.     end
  127. end
  128.  
  129. function InitObjects()
  130.     local holder = CreateFrame("Frame", "AuraTrackerHolder", UIParent);
  131.     holder:SetPoint(unpack(point));
  132.     holder:SetSize((size + gap) * max - gap, size);
  133.  
  134.     holder.button = {};
  135.  
  136.     for i = 1, max do
  137.         local button = CreateFrame("Frame", "$parentButton" .. i, holder);
  138.         button:Hide();
  139.  
  140.         if i == 1 then
  141.             button:SetPoint("LEFT");
  142.         else
  143.             button:SetPoint("LEFT", holder.button[i - 1], "RIGHT", gap, 0);
  144.         end
  145.  
  146.         button:SetSize(size, size);
  147.  
  148.         local cd = CreateFrame("Cooldown", "$parentCooldown", button, "CooldownFrameTemplate");
  149.         cd:SetAllPoints(button);
  150.  
  151.         local icon = button:CreateTexture(nil, "BORDER");
  152.         icon:SetAllPoints(button);
  153.  
  154.         local count = button:CreateFontString(nil, "OVERLAY");
  155.         count:SetFontObject(NumberFontNormal);
  156.         count:SetPoint("TOP", button, "BOTTOM", 0, -2);
  157.  
  158.         holder.button[i] = button;
  159.  
  160.         button.cd = cd;
  161.         button.icon = icon;
  162.         button.count = count;
  163.     end
  164.  
  165.     AuraTrack.holder = holder;
  166. end

I am pretty happy with the rest of parts, but :UNIT_AURA() function is... meh.......

Even if I've left some comments there, pretty sure I would forget what I have done there with all those for loops.

Could I ask some helps?!

Thank you.

Last edited by Layback_ : 05-07-17 at 10:03 AM.
  Reply With Quote
05-07-17, 12:38 PM   #2
Kakjens
A Cliff Giant
Join Date: Apr 2017
Posts: 75
For me it seems like AuraTrack:UNIT_AURA(...) can be split into two functions.
  Reply With Quote
05-07-17, 05:13 PM   #3
MunkDev
A Scalebane Royal Guard
 
MunkDev's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2015
Posts: 424
(1) You're doing checkups on tables that may not exist. If you don't intend to add tables for every class and spec, anyone else using your addon that is not a destruction warlock will get errors.

(2) What's the point in your holder frame and not just using the AuraTrack frame you already created to show your auras?

(3) You don't need to upvalue the global environment just to grab a few functions. This process is quick and painless and only happens once on login anyway.

(4) Unless there's a specific reason why you're using a set max of 6 auras, you can use a frame factory / frame pool to spawn your trackers instead of creating them on load. I'll show you an example of how this works with the native implementation already in the UI.

Lua Code:
  1. -- Upvalue variables
  2. local GetSpellInfo = GetSpellInfo;
  3. local UnitAura = UnitAura;
  4.  
  5. -- Local variables
  6. local size = 40;
  7. local point = {"CENTER", UIParent, "CENTER", 0, -110};
  8. local gap = 2;
  9.  
  10. local buffIDList = {
  11.     ["WARLOCK"] = {
  12.         [3] = { -- Destruction
  13.             235156,
  14.             215165,
  15.             119899,
  16.         },
  17.     },
  18. };
  19.  
  20. -- AuraTrack
  21. local AuraTrack = CreateFrame("Frame", "AuraTrack", UIParent);
  22. AuraTrack.numTotalObjects = 0;
  23. AuraTrack:SetPoint(unpack(point));
  24. AuraTrack:RegisterEvent("ACTIVE_TALENT_GROUP_CHANGED");
  25. AuraTrack:RegisterEvent("PLAYER_LOGIN");
  26. AuraTrack:RegisterEvent("ADDON_LOADED");
  27. AuraTrack:SetScript("OnEvent", function(self, event, ...)
  28.     self[event](self, ...);
  29. end);
  30.  
  31.  
  32. function AuraTrack:UNIT_AURA(...)
  33.     -- Release all the currently active objects
  34.     -- See the :ResetTracker function for what it does to individual objects.
  35.     self:ReleaseAll();
  36.  
  37.     -- Get the buffs to track for the current spec
  38.     -- Assert a table exists for the player's class and spec
  39.     local buffIDs = self.buffIDList;
  40.  
  41.     if buffIDs then
  42.  
  43.         -- Iterate over buff IDs
  44.         local prevButton;
  45.         for i, buffID in pairs(buffIDs) do
  46.             local spellName = GetSpellInfo(buffID);
  47.             local _, _, icon, count, _, duration, expires = UnitAura(..., spellName);
  48.             if duration and duration > 0 then
  49.                 -- Acquire a button from the object pool
  50.                 local button = self:Acquire();
  51.  
  52.                 -- Assign your stuff
  53.                 button.spellID = buffID; -- not sure this is necessary with this approach.
  54.                 button.cd:SetCooldown(expires - duration, duration);
  55.                 button.icon:SetTexture(icon);
  56.                 button.count:SetText((count and count > 0) and count or "");
  57.                
  58.                 -- Show and set the point for the button
  59.                 button:Show();
  60.                 if prevButton then
  61.                     button:SetPoint("LEFT", prevButton, "RIGHT", gap, 0);
  62.                 else
  63.                     button:SetPoint("LEFT");
  64.                 end
  65.  
  66.                 -- Reference this button so the next one knows where to anchor
  67.                 prevButton = button;
  68.             end
  69.         end
  70.     end
  71.  
  72.     -- Update width to match active objects.
  73.     -- numActiveObjects is provided by the object pool mixin.
  74.     self:SetSize(((size + gap) * self.numActiveObjects) - gap, size);
  75. end
  76.  
  77. -- Update spec and the buff table when the spec actually changes
  78. function AuraTrack:ACTIVE_TALENT_GROUP_CHANGED()
  79.     local _, class = UnitClass("player");
  80.     local spec = GetSpecialization();
  81.     self.buffIDList = buffIDList[class] and buffIDList[class][spec];
  82. end
  83.  
  84. function AuraTrack:ADDON_LOADED(...)
  85.     if ... == "MyAuraTracker" then
  86.         self.ADDON_LOADED = nil;
  87.         self:UnregisterEvent("ADDON_LOADED");
  88.         self:RegisterUnitEvent("UNIT_AURA", "player");
  89.     end
  90. end
  91.  
  92. function AuraTrack:PLAYER_LOGIN()
  93.     -- Run the spec check on login
  94.     self:ACTIVE_TALENT_GROUP_CHANGED();
  95.     self:UnregisterEvent("PLAYER_LOGIN");
  96.     self.PLAYER_LOGIN = nil;
  97. end
  98.  
  99. -- Related to creating and recycling active trackers
  100. function AuraTrack:CreateTracker()
  101.     -- This counter is only here to generate non-garbage frame names
  102.     self.numTotalObjects = self.numTotalObjects + 1;
  103.  
  104.     local id = self.numTotalObjects;
  105.     local button = CreateFrame("Frame", "$parentButton"..id, self);
  106.  
  107.     button:Hide();
  108.     button:SetSize(size, size);
  109.  
  110.     local cd = CreateFrame("Cooldown", "$parentCooldown", button, "CooldownFrameTemplate");
  111.     cd:SetAllPoints(button);
  112.  
  113.     local icon = button:CreateTexture(nil, "BORDER");
  114.     icon:SetAllPoints(button);
  115.  
  116.     local count = button:CreateFontString(nil, "OVERLAY");
  117.     count:SetFontObject(NumberFontNormal);
  118.     count:SetPoint("TOP", button, "BOTTOM", 0, -2);
  119.  
  120.     button.cd = cd;
  121.     button.icon = icon;
  122.     button.count = count;
  123.  
  124.     return button;
  125. end
  126.  
  127. function AuraTrack:ResetTracker(button)
  128.     button.spellID = nil;
  129.     button:ClearAllPoints();
  130.     button:Hide();
  131. end
  132.  
  133. -- Mix in the object pool functions
  134. Mixin(AuraTrack, ObjectPoolMixin);
  135. -- Give the object pool creation and resetting functions
  136. AuraTrack:OnLoad(AuraTrack.CreateTracker, AuraTrack.ResetTracker);
__________________

Last edited by MunkDev : 05-07-17 at 05:35 PM.
  Reply With Quote
05-07-17, 05:35 PM   #4
Lombra
A Molten Giant
 
Lombra's Avatar
AddOn Author - Click to view addons
Join Date: Nov 2006
Posts: 554
Originally Posted by MunkDev View Post
(3) You don't need to upvalue the global environment just to grab a few functions. This process is quick and painless and only happens once on login anyway.
Pretty sure it's a net loss, even.
__________________
Grab your sword and fight the Horde!
  Reply With Quote
05-07-17, 07:08 PM   #5
Layback_
An Onyxian Warder
Join Date: Feb 2016
Posts: 356
Hi MunkDev,

Your example is awesome !!!!

Originally Posted by MunkDev View Post
(1) You're doing checkups on tables that may not exist. If you don't intend to add tables for every class and spec, anyone else using your addon that is not a destruction warlock will get errors.
You are right! It was to show my intention of this addon and id for each class and spec are not listed there, yet

If possible, I am planning to create an interface options panel to add the id for each class and spec.

Originally Posted by MunkDev View Post
(2) What's the point in your holder frame and not just using the AuraTrack frame you already created to show your auras?
Sorry for the lacking explanation.

The actual project uses Ace3.0 and AuraTrack in that case is actually an addon of Ace3.0 (not frame).

I totally forgot to clarify regarding that

Originally Posted by MunkDev View Post
(3) You don't need to upvalue the global environment just to grab a few functions. This process is quick and painless and only happens once on login anyway.


Originally Posted by MunkDev View Post
(4) Unless there's a specific reason why you're using a set max of 6 auras, you can use a frame factory / frame pool to spawn your trackers instead of creating them on load. I'll show you an example of how this works with the native implementation already in the UI.
There was no specific reason to lock max frame to 6, but I was just trying to match its width to one of my center bar

In the future, if I want to limit the max frame to 6, I guess I could just put if statement at the top of :UNIT_AURA() function, am I right?

Code:
function AuraTrack:UNIT_AURA(...)
	if self.numTotalObjects >= 6 then
		return;
	end

	-- rest of your code
end


I have skimmed through the APIs and there are new terms (and functions) in your example that I am not familiar with.
  • Mixin
  • ObjectPoolMixin
  • :OnLoad()
  • :ReleaseAll()
  • :Acquire()

So, you are inheriting ObjectPoolMixin's functions to AuraTracker via Mixin which are OnLoad, ReleaseAll and Acquire, right?

Learning new stuff is always exciting

*EDIT: Guess I found a reference here

*EDIT2: Are there any other good Mixin that is good to know with?

Last edited by Layback_ : 05-07-17 at 07:57 PM.
  Reply With Quote
05-07-17, 08:07 PM   #6
MunkDev
A Scalebane Royal Guard
 
MunkDev's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2015
Posts: 424
Mixin literally stands for "mix in", as in mixing in stuff from another table into your own table. It's a good way to preserve memory, because it utilises the same basic functions that are useful in many different cases. It's essentially an implementation of inheritance and object-oriented programming, albeit less bloated.

There's also a function called CreateFramePool, which returns a table that can be used to recycle objects that are displayed dynamically. A typical case for that is when you don't know how many objects you need. The downside is that it expects an XML template for the type of frame you want to create a pool for.

In the example I posted, I used a simpler solution, which is to only use the downscaled ObjectPoolMixin. This is the basis of a frame pool, but you supply the functions to create and reset your objects yourself. This is useful for when you're creating your objects in pure Lua.

Generally, you probably shouldn't go looking for mixins unless you have a distinct need for them. If you do insist, however, you can use this script to list all existing mixins in your chat:
Lua Code:
  1. /run for k in pairs(_G) do if k:match("Mixin") then print(k) end end
__________________
  Reply With Quote
05-07-17, 08:26 PM   #7
Layback_
An Onyxian Warder
Join Date: Feb 2016
Posts: 356
Originally Posted by MunkDev View Post
Generally, you probably shouldn't go looking for mixins unless you have a distinct need for them. If you do insist, however, you can use this script to list all existing mixins in your chat:
Lua Code:
  1. /run for k in pairs(_G) do if k:match("Mixin") then print(k) end end
Wow... there are tons of Mixins haha!!

Thank you so much for your detailed explanation!

I really appreciate your help
  Reply With Quote
05-08-17, 06:32 AM   #8
Kkthnx
A Cobalt Mageweaver
 
Kkthnx's Avatar
AddOn Author - Click to view addons
Join Date: Aug 2011
Posts: 243
Originally Posted by MunkDev View Post
(3) You don't need to upvalue the global environment just to grab a few functions. This process is quick and painless and only happens once on login anyway.
Originally Posted by Lombra View Post
Pretty sure it's a net loss, even.
I'd love to see more info on this. I have learned that is it is faster.

For example

Lua Code:
  1. local _G = _G

then follow suit

Lua Code:
  1. local GetCVar = _G.GetCVar

Is this bad practice? If so I'd love more info on this. Everyone has their ways.

So telling him he doesn't need to do this is right or wrong here or are you teaching him what you have learned.

Lua Code:
  1. -- WoW Lua
  2. local _G = _G
  3.  
  4. -- Wow API
  5. local GetSpecialization = _G.GetSpecialization
  6. local GetSpellInfo = _G.GetSpellInfo
__________________
Success isn't what you've done compared to others. Success is what you've done compared to what you were made to do.

Last edited by Kkthnx : 05-08-17 at 06:40 AM.
  Reply With Quote
05-08-17, 06:54 AM   #9
lightspark
A Rage Talon Dragon Guard
 
lightspark's Avatar
AddOn Author - Click to view addons
Join Date: Sep 2012
Posts: 326
The only reason why I do local _G = getfenv(0) is my paranoia o_O

I've seen addons that do some weird manipulations w/ environments, so _G may end up being something else, and a check like _G == getfenv(0) will return false.

IMHO, if you have no paranoia and don't call global functions from inside OnUpdate and prob OnEvent funcs, only if you register for REALLY spammy events, e.g., CLEU, personally, I consider UNIT_POWER_FREQUENT spammy too, there's no point in creating upvalues for pretty much anything.

Another reason might be to please one's Lua linter
__________________

Last edited by lightspark : 05-08-17 at 06:58 AM.
  Reply With Quote
05-08-17, 07:00 AM   #10
Kkthnx
A Cobalt Mageweaver
 
Kkthnx's Avatar
AddOn Author - Click to view addons
Join Date: Aug 2011
Posts: 243
Originally Posted by lightspark View Post
The only reason why I do local _G = getfenv(0) is my paranoia o_O

I've seen addons that do some weird manipulations w/ environments, so _G may end up being something else, and a check like _G == getfenv(0) will return false.

IMHO, if you have no paranoia and don't call global functions from inside OnUpdate and prob OnEvent funcs, only if you register for REALLY spammy events, e.g., CLEU, there's no point in creating upvalues for pretty much anything.

Another reason might be to please one's Lua linter
Lmao @ please one's Lua linter! This is so true and has become a thing for me. I use Atom text editor and https://atom.io/packages/linter-lua-findglobals so I guess I am just so used to pleasing it

Also never thought of the whole _G = getfenv(0) deal. Incoming paranoia?
__________________
Success isn't what you've done compared to others. Success is what you've done compared to what you were made to do.
  Reply With Quote
05-08-17, 07:05 AM   #11
lightspark
A Rage Talon Dragon Guard
 
lightspark's Avatar
AddOn Author - Click to view addons
Join Date: Sep 2012
Posts: 326
Originally Posted by Kkthnx View Post
Lmao @ please one's Lua linter! This is so true and has become a thing for me. I use Atom text editor and https://atom.io/packages/linter-lua-findglobals so I guess I am just so used to pleasing it

Also never thought of the whole _G = getfenv(0) deal. Incoming paranoia?
I call it _G-fetish

But in most cases upvalues and/or accessing globals via _G are a personal preference or even a style, people like weird things o_O
__________________
  Reply With Quote
05-08-17, 08:40 AM   #12
MunkDev
A Scalebane Royal Guard
 
MunkDev's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2015
Posts: 424
Originally Posted by Kkthnx View Post
Is this bad practice? If so I'd love more info on this. Everyone has their ways.

So telling him he doesn't need to do this is right or wrong here or are you teaching him what you have learned.

Lua Code:
  1. -- WoW Lua
  2. local _G = _G
  3.  
  4. -- Wow API
  5. local GetSpecialization = _G.GetSpecialization
  6. local GetSpellInfo = _G.GetSpellInfo
I'm not saying it's bad practice, I'm saying it's pointless. Lua in WoW assumes global environment access if the variable name you're looking for doesn't exist in your current scope. Localising those functions only entails two instances of table access. Why store a pointer to the global environment for the remainder of your game session when you're not even using it for anything?

The performance gain is negligible at best. The only recurring functions that are used in loops in response to a frequent event are the two functions I listed at the top. If you're doing a ton of lookups very frequently, it might be wise to upvalue functions or entire tables in that case.

Personally, I don't upvalue the global environment because it produces uglier code.
__________________

Last edited by MunkDev : 05-08-17 at 08:58 AM.
  Reply With Quote
05-08-17, 10:38 AM   #13
Ketho
A Molten Giant
 
Ketho's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2010
Posts: 864
There were those old threads about _G upvaluing if that helps
  Reply With Quote
05-08-17, 12:39 PM   #14
Lombra
A Molten Giant
 
Lombra's Avatar
AddOn Author - Click to view addons
Join Date: Nov 2006
Posts: 554
Originally Posted by Kkthnx View Post
I'd love to see more info on this. I have learned that is it is faster.
I'm not saying you did, but it's important not to take my quote out of context. I referred to this code sample, specifically. The call to getfenv() will take "much" longer than what you gained from upvaluing _G. _G = _G is better, in that case. Still very pointless.
Originally Posted by lightspark View Post
The only reason why I do local _G = getfenv(0) is my paranoia o_O

I've seen addons that do some weird manipulations w/ environments, so _G may end up being something else, and a check like _G == getfenv(0) will return false.
I mean, sure you can be careful, but there is such a thing as being too careful to the point of it just being foolish. "No offence". If addons are reassigning the global _G, at that point they're practically malware.


People need to understand what kind of gains we are talking about here. Some results were presented in one of the threads Ketho linked. I'd like to provide some more.

I tested the following cases using debugprofilestop(), which is very precise.

Over a hundred thousand iterations, calling UnitAura without any upvalues takes around 52 milliseconds.
Code:
for i = 1, 1e5 do
   local a = UnitAura("player", 1)
end
Doing the same with an upvalued UnitAura takes just under 51 ms.
Code:
local UnitAura = UnitAura
for i = 1, 1e5 do
   local a = UnitAura("player", 1)
end

As for assigning values, we're upping the iteration count to one million for increased accuracy. Upvaluing without using _G takes 15.5 ms.
Code:
for i = 1, 1e6 do
   local a = UnitAura
end
Upvaluing using _G, but without first having upvalued _G it self takes 26 ms, which is significantly slower. There's never any reason to do this thinking it's faster. (that's not the net loss I was talking about, though)
Code:
for i = 1, 1e6 do
   local a = _G.UnitAura
end
Doing the same with an upvalued _G is... actually still slower, for some reason. 15.7 ms. When I tested on PTR last night it was slightly faster. Possibly related to size of _G? Though I would've thought doing a global lookup would be affect by the size of _G in the same way.
Code:
local _G = _G
for i = 1, 1e6 do
   local a = _G.UnitAura
end
This is what I was referring to. Upvaluing _G to upvalue a single value through that is a net loss due to having perform a global lookup to get _G, and then a table lookup in _G to get UnitAura, as opposed to just doing a global lookup for UnitAura. This took 26 ms. Of course, the gain should be greater the more values you're upvaluing, though the above seems to imply the opposite... I'm not sure if there is some underlying caching going on that's skewing the results.
Code:
for i = 1, 1e6 do
   local _G = _G
   local a = _G.UnitAura
end
For reference, replaing _G with getfenv(0) in the above code puts the result at 123 ms.

Do with this info as you will.
Here is the exact test suite I'm using. Feel free to try to produce your own results.
Code:
local t = debugprofilestop

local ct = t()

for i = 1, 1e6 do
   local _G = getfenv(0)
   local a = _G.UnitAura
end

ct = t() - ct

print(format("Execution time: %s ms.", ct))
__________________
Grab your sword and fight the Horde!
  Reply With Quote
05-08-17, 01:05 PM   #15
lightspark
A Rage Talon Dragon Guard
 
lightspark's Avatar
AddOn Author - Click to view addons
Join Date: Sep 2012
Posts: 326
Originally Posted by Lombra View Post
I mean, sure you can be careful, but there is such a thing as being too careful to the point of it just being foolish. "No offence". If addons are reassigning the global _G, at that point they're practically malware.
That's why I said what I said, I do it because of my paranoia, and I have it because I've seen shit. Being paranoid is kinda foolish.

I agree w/ you and I also said that there's almost no need to create any upvalues in 99.99% of cases. Basically, my comment was "do as I say, not as I do"

P.S. I prob should start fighting _G-paranoia though....
__________________

Last edited by lightspark : 05-08-17 at 01:20 PM.
  Reply With Quote
05-08-17, 02:09 PM   #16
Lombra
A Molten Giant
 
Lombra's Avatar
AddOn Author - Click to view addons
Join Date: Nov 2006
Posts: 554
Fair enough.
__________________
Grab your sword and fight the Horde!
  Reply With Quote
05-08-17, 03:51 PM   #17
MunkDev
A Scalebane Royal Guard
 
MunkDev's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2015
Posts: 424
Originally Posted by lightspark View Post
That's why I said what I said, I do it because of my paranoia, and I have it because I've seen shit. Being paranoid is kinda foolish.
You're also not building software for NASA's rocket computers. If someone produces shitty, incompatible code in their add-ons that completely dismantle the entire interface, they'll fail automatically because people will not download them.
__________________
  Reply With Quote
05-08-17, 03:53 PM   #18
Kanegasi
A Rage Talon Dragon Guard
 
Kanegasi's Avatar
AddOn Author - Click to view addons
Join Date: Apr 2007
Posts: 337
Originally Posted by lightspark View Post
That's why I said what I said, I do it because of my paranoia, and I have it because I've seen shit. Being paranoid is kinda foolish.

I agree w/ you and I also said that there's almost no need to create any upvalues in 99.99% of cases. Basically, my comment was "do as I say, not as I do"

P.S. I prob should start fighting _G-paranoia though....
I can fully understand code paranoia. During a testing phase of Decliner, the friend helping me said it didn't work. After some investigation, having them use /dump on my main player check function returned nil, but it returned a function for me. I double-checked they were using the exact same /dump string and that my addon was loaded.

My answer was to move the check function to a local and then make a global dummy function that returns the check function. The worst part was the integration into a timer loop I use to check the global dummy every five seconds and reinitialize it if it's nil, while trying to out the addon deleting it with ADDON_LOADED. I recently removed that part of the timer loop actually, but I still hide the main check function locally, while the global dummy is just an API thing I keep.

Paranoia aside, I still learned that I shouldn't have important functions outside my addon's scope.

Last edited by Kanegasi : 05-08-17 at 03:55 PM.
  Reply With Quote
05-09-17, 01:35 AM   #19
lightspark
A Rage Talon Dragon Guard
 
lightspark's Avatar
AddOn Author - Click to view addons
Join Date: Sep 2012
Posts: 326
Originally Posted by MunkDev View Post
You're also not building software for NASA's rocket computers.
Sure. I fully agree

Originally Posted by MunkDev View Post
If someone produces shitty, incompatible code in their add-ons that completely dismantle the entire interface, they'll fail automatically because people will not download them.
Oh, people do download a lot of shit, they download and use the worst addons ever quite happily, and then they report "bugs" to other addons' devs, and then those devs waste their time trying to reproduce said "bugs", whereas they're caused by other addons that do some mind-boggling stuff w/ API and/or environments

I once wasted a lot of time doing so...
__________________
  Reply With Quote
05-09-17, 01:37 AM   #20
lightspark
A Rage Talon Dragon Guard
 
lightspark's Avatar
AddOn Author - Click to view addons
Join Date: Sep 2012
Posts: 326
Originally Posted by Kanegasi View Post
I can fully understand code paranoia. During a testing phase of Decliner, the friend helping me said it didn't work. After some investigation, having them use /dump on my main player check function returned nil, but it returned a function for me. I double-checked they were using the exact same /dump string and that my addon was loaded.

My answer was to move the check function to a local and then make a global dummy function that returns the check function. The worst part was the integration into a timer loop I use to check the global dummy every five seconds and reinitialize it if it's nil, while trying to out the addon deleting it with ADDON_LOADED. I recently removed that part of the timer loop actually, but I still hide the main check function locally, while the global dummy is just an API thing I keep.

Paranoia aside, I still learned that I shouldn't have important functions outside my addon's scope.
Been there, done that T_T Some devs are... Ugh
__________________
  Reply With Quote

WoWInterface » Developer Discussions » Lua/XML Help » How could this be written neater?

Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

vB code is On
Smilies are On
[IMG] code is On
HTML code is Off