Optimizing/checking bad coding 2 small addons

[ramzax]

Hello, I'd like some help optimizing or checking bad coding in 2 small addons I wrote earlier today.

The first one announces cooldowns/spells used, but I don't like how I'm currently checking spells, feels really clunky so I wonder if there's a better way to do it:

--------------------------------------
--cooldownAnnouncer
--------------------------------------

---------------CONFIG---------------
--  Channel
local channel = "SAY";

--  Spells to check
local cooldowns = {
    ["MAGE"] = {61316}, --Dalaran Brilliance as an example
};

---------------CONFIG---------------

-- Player
local _, playerClass = UnitClass("player");

--  Create frame
local cooldownAnnouncer = CreateFrame("Frame")

--  Events to listen
cooldownAnnouncer:RegisterEvent("UNIT_SPELLCAST_SENT")
cooldownAnnouncer:RegisterEvent("UNIT_SPELLCAST_SUCCEEDED")

--  Do stuff
cooldownAnnouncer:SetScript("OnEvent",
    function(self, event, unit, spell)
        if unit == "player" then
            for i=1,#cooldowns[playerClass] do
                if spell == GetSpellInfo(cooldowns[playerClass][i]) then
                    if event == "UNIT_SPELLCAST_SUCCEEDED" then
                        SendChatMessage(spell, channel);
                    end
                end
            end
        end
end)

The second one sets raid icons to the player and party (for arena) based on if a spell cast is successful (ex. a buff for when rogues vanish, displacer beast bug, etc). I reused code I found in this thread.

--------------------------------------
--raidIcons
--------------------------------------

--  Spells to check
local spells = {
    ["DEATHKNIGHT"]     =   GetSpellInfo(57330),        --  Horn of Winter
    ["DRUID"]           =   GetSpellInfo(1126),         --  Mark of the Wild
    ["HUNTER"]          =   GetSpellInfo(13165),        --  Aspect of the Hawk
    ["MAGE"]            =   GetSpellInfo(61316),        --  Dalaran Brilliance
    ["MONK"]            =   GetSpellInfo(115921),       --  Legacy of the White Tiger
    ["PALADIN"]         =   GetSpellInfo(20217),        --  Blessing of Kings
    ["PRIEST"]          =   GetSpellInfo(21562),        --  Power Word: Fortitude
    ["ROGUE"]           =   GetSpellInfo(1784),         --  Stealth
    ["SHAMAN"]          =   GetSpellInfo(52127),        --  Water Shield
    ["WARLOCK"]         =   GetSpellInfo(109773),       --  Dark Intent
    ["WARRIOR"]         =   GetSpellInfo(6673),         --  Battle Shout
}

-- 1 = Yellow 4-point Star      3 = Purple Diamond      5 = White Crescent Moon     7 = Red "X" Cross
-- 2 = Orange circle            4 = Green Triangle      6 = Blue Square             8 = White Skull

--  Icons to set
local icons = {
    ["DEATHKNIGHT"]     =   {DEATHKNIGHT=8,DRUID=2,HUNTER=4,PALADIN=3,MAGE=5,MONK=4,PRIEST=5,ROGUE=1,SHAMAN=6,WARLOCK=3,WARRIOR=7},
    ["DRUID"]           =   {DEATHKNIGHT=8,DRUID=2,HUNTER=4,PALADIN=3,MAGE=5,MONK=4,PRIEST=5,ROGUE=1,SHAMAN=6,WARLOCK=3,WARRIOR=7},
    ["HUNTER"]          =   {DEATHKNIGHT=8,DRUID=2,HUNTER=4,PALADIN=3,MAGE=5,MONK=2,PRIEST=5,ROGUE=1,SHAMAN=6,WARLOCK=3,WARRIOR=7},
    ["MAGE"]            =   {DEATHKNIGHT=8,DRUID=2,HUNTER=4,PALADIN=3,MAGE=5,MONK=4,PRIEST=6,ROGUE=1,SHAMAN=6,WARLOCK=3,WARRIOR=7},
    ["MONK"]            =   {DEATHKNIGHT=8,DRUID=2,HUNTER=4,PALADIN=3,MAGE=5,MONK=4,PRIEST=5,ROGUE=1,SHAMAN=6,WARLOCK=3,WARRIOR=7},
    ["PALADIN"]         =   {DEATHKNIGHT=8,DRUID=2,HUNTER=4,PALADIN=6,MAGE=5,MONK=4,PRIEST=5,ROGUE=1,SHAMAN=6,WARLOCK=3,WARRIOR=7},
    ["PRIEST"]          =   {DEATHKNIGHT=8,DRUID=2,HUNTER=4,PALADIN=3,MAGE=6,MONK=4,PRIEST=5,ROGUE=1,SHAMAN=6,WARLOCK=3,WARRIOR=7},
    ["ROGUE"]           =   {DEATHKNIGHT=8,DRUID=2,HUNTER=4,PALADIN=3,MAGE=5,MONK=4,PRIEST=5,ROGUE=1,SHAMAN=6,WARLOCK=3,WARRIOR=7},
    ["SHAMAN"]          =   {DEATHKNIGHT=8,DRUID=2,HUNTER=4,PALADIN=3,MAGE=5,MONK=4,PRIEST=5,ROGUE=1,SHAMAN=6,WARLOCK=3,WARRIOR=7},
    ["WARLOCK"]         =   {DEATHKNIGHT=8,DRUID=2,HUNTER=4,PALADIN=6,MAGE=5,MONK=4,PRIEST=5,ROGUE=1,SHAMAN=6,WARLOCK=3,WARRIOR=7},
    ["WARRIOR"]         =   {DEATHKNIGHT=8,DRUID=2,HUNTER=4,PALADIN=3,MAGE=5,MONK=4,PRIEST=5,ROGUE=1,SHAMAN=6,WARLOCK=3,WARRIOR=7},
}

--  Create frame
local raidIcons = CreateFrame("Frame")

--  Events to listen
raidIcons:RegisterEvent("UNIT_SPELLCAST_SENT")
raidIcons:RegisterEvent("UNIT_SPELLCAST_SUCCEEDED")

--  Do stuff
raidIcons:SetScript("OnEvent",
    function(self, event, unit, spell)
        if not UnitIsGroupLeader("player") and not UnitIsGroupAssistant("player") then return end
        if GetNumGroupMembers()>5 then return end
        local _, playerClass = UnitClass("player")
        if unit == "player" and spell == spells[playerClass] then
            if event == "UNIT_SPELLCAST_SUCCEEDED" and GetRaidTargetIndex("player")==nil then
                members = GetNumGroupMembers()
                icon = icons[playerClass]
                SetRaidTarget('player',icon[playerClass])
                for i=1,(members-1) do
                    SetRaidTarget('party'..i,icon[select(2,UnitClass('party'..i))])
                end
            end
        end
end)

[Lombra]

Hi,

For the first one, you can start by using hash tables instead of arrays. Use the value as the key, and true as the value. (or any other non false value) This will let you do a simple table key lookup to determine whether a given value exists, rather than looping over the whole table.

local cooldowns = {
	["MAGE"] = {
		[61316] = true, --Dalaran Brilliance as an example
	},
};

if cooldowns[61316] then
	print("this spell exists")
end

A neat thing was implemented some time ago for unit events, that lets you register events for certain units only. Useful if you're only interested in a single unit. (or two, which also works) Then you don't have to check whether unit == "player" every time. Just change the registration and remove the unit check. Everything else remains the same.

cooldownAnnouncer:RegisterUnitEvent("UNIT_SPELLCAST_SENT", "player")
cooldownAnnouncer:RegisterUnitEvent("UNIT_SPELLCAST_SUCCEEDED", "player")

And finally, these events pass a spell ID argument in addition to spell name, which actually is a prerequisite for part one to work, and you won't have to use GetSpellInfo to get the spell name from the predetermined spells.

--------------------------------------
--cooldownAnnouncer
--------------------------------------

---------------CONFIG---------------
--  Channel
local channel = "SAY";

--  Spells to check
local cooldowns = {
	["MAGE"] = {
		[61316] = true, --Dalaran Brilliance as an example
	},
};

---------------CONFIG---------------

-- Player
local _, playerClass = UnitClass("player");

--  Create frame
local cooldownAnnouncer = CreateFrame("Frame")

--  Events to listen
cooldownAnnouncer:RegisterUnitEvent("UNIT_SPELLCAST_SENT", "player")
cooldownAnnouncer:RegisterUnitEvent("UNIT_SPELLCAST_SUCCEEDED", "player")

--  Do stuff
cooldownAnnouncer:SetScript("OnEvent", function(self, event, unit, spell, _, _, spellID)
	if cooldowns[playerClass][spellID] then
		if event == "UNIT_SPELLCAST_SUCCEEDED" then
			SendChatMessage(spell, channel);
		end
	end
end)

[ramzax]

thanks a lot, didn't know you could only check events for 1 unit, I'll keep that in mind