View Single Post
07-25-13, 09:18 PM   #24
Phanx
Cat.
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 5,617
Just from a quick glance, a few suggestions:

Code:
function events:COMBAT_LOG_EVENT_UNFILTERED(event, ...)
	local _, eventType, _, sourceGUID, sourceName, _, _, destGUID, destName, _, _ = ...
Since you now have a dedicated function for this event, you might as well just name the variables directly as the function receives them, rather than receiving them as a vararg and naming them later. There's also no need to capture those last two underscore variables, unless you're actually using them, in which case you should give them descriptive names so it's clear they're not "junk" like the other underscores.

Code:
function events:COMBAT_LOG_EVENT_UNFILTERED(event, _, eventType, _, sourceGUID, sourceName, _, _, destGUID, destName)
In the same function, you look up bossNPCID[mobIDa] and bossNPCID[mobIDb] multiple times each. You should look them up just once, and store the values in variables, to avoid extra table lookups. Similarly, always check for return conditions before doing other work:

[code]
function events:COMBAT_LOG_EVENT_UNFILTERED(event, _, eventType, _, sourceGUID, sourceName, _, _, destGUID, destName)
local mobIDa, mobIDb = mobIDfromGUID[sourceGUID], mobIDfromGUID[destGUID]
local bossIDa, bossIDb = bossNPCID[mobIDa], bossNPCID[mobIDb]

-- Quit if nokill is set, or the NPC isn't a listed boss:
if nokill or (not bossIDa and not bossIDb) then
return
-- ...or if the boss died:
elseif eventType == "UNIT_DIED" and bossIDb then
nokill, mobName = nil, nil
loottrigger = true
return self:RegisterEvent("LOOT_OPENED")
end

-- Get mobName from either sourceName or destName.
-- Since the NPC is definitely a boss at this point, it must be one
-- or the other, so no need to explicitly check bossIDb too.
mobName = bossIDa and sourceName or destName

-- insert other code here, starting with the RegisterEvent line
end

Finally, the loot-related block in your CLEU handler looks a bit off. As written, you will never change the loot method unless it's set to something other than "master" on the very first time that code runs. After the first time, the "method" and "threshold" variables are already set, so you'll just return out.

If that's not intentional, you should fix it. I can't really think of a reason to check and set the method/threshold variables inside CLEU anyway -- any changes to the loot method or threshold will trigger PARTY_LOOT_METHOD_CHANGED, which you're already handling separately, so that should be the only place you need to set those variables.

If you're seeing issues while logging in in a group (eg. PLMC doesn't fire at login so you don't know anything about the loot method when CLEU starts firing), add a PLAYER_LOGIN handler that calls your PARTY_LOOT_METHOD_CHANGED method (just like your PARTY_LEADER_CHANGED handler calls your PLAYER_ENTERING_WORLD method).

Also:

Code:
self:UnregisterEvent(self)
I don't know what you were trying to do here, but this line will cause an error, because "self" in that scope is a pointer to the "events" frame, and the UnregisterEvent method requires a string, not a frame pointer. I think you may have meant to unregister the currently handled event, in which case you should change this line to:

Code:
self:UnregisterEvent(event)
__________________
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