View Single Post
07-23-13, 11:21 PM   #18
Phanx
Cat.
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 5,617
Originally Posted by Niketa View Post
Is this a step in the right direction or am I still way off?
That's looking a lot better. Just a couple suggestions:

1. Your event handler should be an if-else chain, not a bunch of separate if blocks. For example, right now, when PLAYER_ENTERING_WORLD fires, the first if block in your function runs -- but then all the other blocks have to be checked, too, even though it's obvious to you as a human that they'll never match that event. Alternatively, add a return inside each if block, so that if the block is run, it will exit the function afterwards instead of going through and checking all the other blocks.

Code:
if event == "PLAYER_ENTERING_WORLD" or event == "PARTY_LEADER_CHANGED" then
   -- do something
-- no "end" here
elseif event == "PLAYER_REGEN_DISABLED" then
   -- do something for entering combat
...
2. Instead of checking if event == X and something_else == Y then, you should just check the event first, and then the other condition. Otherwise, even if the event is X, your code is still running through all the other possibilities, when you actually want it to just stop executing the function if the event is X but the other condition doesn't match.

Don't:
Code:
if event == "COMBAT_LOG_EVENT_UNFILTERED" and eventType ~= "SPELL_AURA_APPLIED" then
   -- do something
end
Do:
Code:
if event == "COMBAT_LOG_EVENT_UNFILTERED" then
   if eventType == "SPELL_AURA_APPLIED" then
      -- don't want this, quit.
      return
   end
   -- do something
end
3. Once you start handling more than 2-3 events, or each event's handling code is more than 5-10 lines, you may want to consider splitting each event's code out into its own function. An easy way to do this is to make all the event handler functions methods on your event handler frame, with the same name as the event they handle, and then just call the right method for whichever event is firing:

Code:
events:RegisterEvent("PLAYER_ENTERING_WORLD")
events:RegisterEvent("PARTY_LEADER_CHANGED")

events:SetScript("OnEvent", function(self, event, ...)
   -- automatically call the method for this event, if it exists
   return self[event] and self[event](self, event, ...)
end)

function events:PLAYER_ENTERING_WORLD(event)
   -- this function gets called when PEW fires
end

function events:PARTY_LEADER_CHANGED(event)
   -- this function gets called when PLC fires
end
For cases where multiple events should run the same code, you don't need to duplicate the function; just create an alias:

Code:
function events:PLAYER_ENTERING_WORLD(event)
   -- this function gets called when PEW *or* PLC fires
end

events.PARTY_LEADER_CHANGED = events.PLAYER_ENTERING_WORLD

Originally Posted by Niketa View Post
(* I just realized I forgot to add in your suggestion about caching the NPCID so I don't keep calling local enemyNPCID. I don't really understand your code for that [as far as metatables go]; would you mind explaining so I actually understand what I'm writing [we don't want another issue like my local variables for CLEU, lol]? *)
Add this in the main chunk of your file, outside of any function, and before you set your OnEvent script:

Code:
local mobIDFromGUID = setmetatable({}, { __index = function(t, guid)
   local mobID = tonumber(strsub(guid, 6, 10), 16)
   t[guid] = mobID
   return mobID
end })
Replace these lines in your CLEU handling code:

Code:
        local enemyNPCIDa = tonumber(strsub(sourceGUID, 6, 10), 16)
        local enemyNPCIDb = tonumber(strsub(destGUID, 6, 10), 16)
With this:

Code:
        local enemyNPCIDa = mobIDFromGUID[sourceGUID]
        local enemyNPCIDb = mobIDFromGUID[destGUID]
How it works:

Code:
local mobIDFromGUID = setmetatable({}, { __index = function(t, guid)
   local mobID = tonumber(strsub(guid, 6, 10), 16)
   t[guid] = mobID
   return mobID
end })
At this point, this is equivalent to:

Code:
local mobIDFromGUID = {}
Then we attach a metatable, which is an additional table that can define special behaviors for the original table to use under certain circumstances:

Code:
setmetatable(mobIDFromGUID, {})
Right now, the metatable is empty, so the table doesn't have any special behaviors. If you try to look up a value that's not in the table, you'll just get nil.

But, we want the table to do something special when we look up a value that isn't actually contained in it, so we'll add an __index key to the metatable, whose value is the function to run when this happens:

Code:
setmetatable(mobIDFromGUID, { __index = function(t, k)
   print("GUID", k, "isn't in the table yet!")
end })
The first argument passed to the __index function is a reference to the mobIDFromGUID table itself. The second argument is the key that was looked up. Now let's make it do something actually useful:

Code:
setmetatable(mobIDFromGUID, { __index = function(t, guid)
   -- Extract the mob ID from the looked-up GUID:
   local mobID = tonumber(strsub(guid, 6, 10), 16)
   -- Store the guid->mobID pair in the table so it can be accessed faster
   -- the next time the same GUID gets looked up:
   t[guid] = mobID
   -- Finally, return the mobID to whatever did the lookup.
   -- As far as the lookup code (eg. mobIDFromGUID(sourceGUID)) is concerned,
   -- the only thing that happened is a regular table lookup.
   return mobID
end })
__________________
Retired author of too many addons.
Message me if you're interested in taking over one of my addons.
Don’t message me about addon bugs or programming questions.
  Reply With Quote