Thread Tools Display Modes
04-10-16, 02:51 AM   #1
gmarco
An Onyxian Warder
 
gmarco's Avatar
AddOn Author - Click to view addons
Join Date: Dec 2009
Posts: 362
best practice

Hi all,

because this is a period quite calm in game for me now I'd like to take some time to fix, rewrite, or coding better some of my addons

I'd like to ask if this is written well (even if it works

Lua Code:
  1. local ADDON = ...
  2.  
  3. -- bla bla bla ..
  4.  
  5. local frame = CreateFrame("FRAME")
  6. frame:RegisterEvent("ZONE_CHANGED_NEW_AREA")
  7. frame:RegisterEvent("PLAYER_ENTERING_WORLD")
  8. frame:RegisterEvent("UPDATE_MOUSEOVER_UNIT")
  9. frame:RegisterEvent("ADDON_LOADED")
  10.  
  11. frame:SetScript("OnEvent", function(self, event, ...)
  12.  
  13.     if event == "ADDON_LOADED" and ... == ADDON then
  14.  
  15.         -- bla bla bla
  16.  
  17.         elseif event=="COMBAT_LOG_EVENT_UNFILTERED" then
  18.    
  19.         timeStamp, event, _, sourceGUID, sourceName, _, _, destGUID, destName, _, _, prefixParam1, prefixParam2, _, suffixParam1, suffixParam2 = ...
  20.  
  21.          -- bla bla bla
  22.  
  23.          end

what I like to understand if the construct "...==ADDON" is fine ... or I should to get the addon name parsing the ... but it could be expensive.


Or I should create a dedicated frame only for the ADDON_LOADED event and then parse in this way ...

Lua Code:
  1. frame:SetScript("OnEvent", function(self, event, addon)
  2.  
  3.     if event == "ADDON_LOADED" and addon == ADDON then


Thanks .
__________________
This is Unix-Land. In quiet nights, you can hear the Windows machines reboot.

Last edited by gmarco : 04-10-16 at 02:55 AM.
  Reply With Quote
04-10-16, 05:41 AM   #2
MunkDev
A Scalebane Royal Guard
 
MunkDev's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2015
Posts: 431
This is such a minuscule detail that it has virtually zero impact on performance and you don't need to worry about it. Creating a frame just for that event is a bad idea, though.
__________________
  Reply With Quote
04-10-16, 08:44 AM   #3
Resike
A Pyroguard Emberseer
AddOn Author - Click to view addons
Join Date: Mar 2010
Posts: 1,290
The issue with your code, is that you register events even before your addon loads which can make a lot of unneccessary calls. In this case specially when you would log into combat.

You should do something like:
Lua Code:
  1. local ADDON = ...
  2.  
  3. local frame = CreateFrame("FRAME")
  4. frame:RegisterEvent("ADDON_LOADED")
  5.  
  6. frame:SetScript("OnEvent", function(self, event, ...)
  7.     if event == "ADDON_LOADED" and ... == ADDON then
  8.         self:RegisterEvent("ZONE_CHANGED_NEW_AREA")
  9.         self:RegisterEvent("PLAYER_ENTERING_WORLD")
  10.         self:RegisterEvent("UPDATE_MOUSEOVER_UNIT")
  11.         self:RegisterEvent("COMBAT_LOG_EVENT_UNFILTERED")
  12.     elseif event == "COMBAT_LOG_EVENT_UNFILTERED" then
  13.        
  14.     end
  15. end)
  Reply With Quote
04-10-16, 05:52 PM   #4
Seerah
Fishing Trainer
 
Seerah's Avatar
WoWInterface Super Mod
Featured
Join Date: Oct 2006
Posts: 10,860
The variables on line 19 should be local.
__________________
"You'd be surprised how many people violate this simple principle every day of their lives and try to fit square pegs into round holes, ignoring the clear reality that Things Are As They Are." -Benjamin Hoff, The Tao of Pooh

  Reply With Quote
04-11-16, 12:06 AM   #5
gmarco
An Onyxian Warder
 
gmarco's Avatar
AddOn Author - Click to view addons
Join Date: Dec 2009
Posts: 362
Hi all

Seerah: yes those values were defined local in the beginning of the file together others local declarations.
I forgot to include before the relevant line.

Lua Code:
  1. local timeStamp, event, sourceGUID, sourceName, destGUID, destName, prefixParam1, prefixParam2, suffixParam1, suffixParam2

I have also modified my addon(s) putting the registering of events after the "ADDON_LOADED" was triggered. (thanks resike and munkdev).

I have also learned, and this was my main question, that the "..." should be checked and evaluate directly, like any others vars, in a IF condition:

Lua Code:
  1. if ... == something then

Really thanks to all for the inputs and help.
__________________
This is Unix-Land. In quiet nights, you can hear the Windows machines reboot.
  Reply With Quote
04-11-16, 04:24 AM   #6
Lombra
A Molten Giant
 
Lombra's Avatar
AddOn Author - Click to view addons
Join Date: Nov 2006
Posts: 554
I don't know that those events would fire before ADDON_LOADED... PLAYER_LOGIN, possibly.

Anyway, you could also use the event table approach, which makes for a, imo, cleaner setup.
Code:
local ADDON = ...

-- bla bla bla ..

local frame = CreateFrame("FRAME")
frame:RegisterEvent("ZONE_CHANGED_NEW_AREA")
frame:RegisterEvent("PLAYER_ENTERING_WORLD")
frame:RegisterEvent("UPDATE_MOUSEOVER_UNIT")
frame:RegisterEvent("ADDON_LOADED")	

frame:SetScript("OnEvent", function(self, event, ...)
	self[event](self, ...)
end

function frame:ADDON_LOADED(addon)
	if addon == ADDON then

        -- bla bla bla

	end
end

function frame:COMBAT_LOG_EVENT_UNFILTERED(timeStamp, event, _, sourceGUID, sourceName, _, _, destGUID, destName, _, _, prefixParam1, prefixParam2, _, suffixParam1, suffixParam2, ...)

         -- bla bla bla

end
__________________
Grab your sword and fight the Horde!
  Reply With Quote
04-13-16, 11:40 PM   #7
gmarco
An Onyxian Warder
 
gmarco's Avatar
AddOn Author - Click to view addons
Join Date: Dec 2009
Posts: 362
Thanks Lombra,

I really learn something new day by day here in this forum (I think this place is the most valuable resource in the net for addons beginner ... and non beginners

Thanks again.
__________________
This is Unix-Land. In quiet nights, you can hear the Windows machines reboot.
  Reply With Quote
06-18-16, 06:14 PM   #8
Aradel
A Defias Bandit
AddOn Author - Click to view addons
Join Date: May 2009
Posts: 3
Hi

I can't provide an answer what's better or worse it really depends on your addon but generally if statements are slow.

If your addon requires hooking up many events to listen from you may benefit from implementing a lookup table for events. I did it as an experiment in one of my addons.

From a low level technical perspective

The problem you could encounter is that as your addon grows you will have one entry point (main function) from your addon which is responsible for delegating the event to an appropriate function that deals with the notification. If you check all of these sequentially using else ifs you could be paying a rather hefty performance penalty. The time it takes to execute the main function for each even fires increases linearly.

I'm in no way a LUA expert and haven't looked into specifics about internal implementations for tables but if you store a function pointer in a table with the eventID as a key you could potentialy have an execution time that is more or less constant excluding the posibilities of cachemisses.

At which point A becomes faster than B would need to be profiled but there are other benefits of organizing the code like this as well.

in practice I'm not sure if there are huge benefits to make here and you need to make a decision if you want to trade memory vs performance

From a readability point of view

You have a short simple to read entry point and if you're consistent in your function naming everything should be easy to find with out having to scroll 50 else if statements

This doesn't really matter if you're the sole developer and know your code base well enough that you're comfortable working in it.
  Reply With Quote

WoWInterface » Developer Discussions » Lua/XML Help » best practice

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