View Single Post
12-11-14, 11:56 AM   #3
Phanx
Cat.
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 5,617
Personally I've never seen the point in splitting each part of the spawn function out into a "make a health bar" function, a "make a power bar" function, a "do stuff for the player unit" and "do stuff for the target unit" functions, etc. You're only running that code once per frame created, so just have it all in the same place.

Code:
local health = CreateFrame("StatusBar", nil, self)
self.Health = health

if unit == "player" then
   -- do stuff with the health bar for the player frame
elseif unit == "target" then
   -- do stuff with the health bar for the target frame
end
Putting each "make a _____" function in its own file, as I've seen some layouts do, is especially wasteful, since loading files from disk is a significant factor in how much time you spend staring at the loading screen while logging in.

----------

This is also pretty inefficient:
Code:
                        BuffBars.customFilter = function(_, _, _, _, _, _, _, _, _, _, _, _, spellID, _, _, _, _, _)
                                if spellID == 97341 or          --Guild Champion
                                        spellID == 93339 or             --Champion
                                        spellID == 93828 or             --Silvermoon Champion
                                        spellID == 93341 or             --Hyjal Champion
                                        spellID == 93347 or             --Therazane Champion
                                        spellID == 97340 or     --Guild Champion
                                        spellID == 93811 or             --Exodar Champion
                                        spellID == 93816 or             --Gilnean Champion
                                        spellID == 72968 or             --Precious's Ribbon
                                        spellID == 93806 or             --Darnassus Champion
                                        spellID == 93368 or             --Wildhammer Champion
                                        spellID == 93337 or             --Ramkahen Champion
                                        spellID == 126434 or    --Tushui Champion
                                        spellID == 57819 or             --Argent Chamption
                                        spellID == 57822 or     --Wyrmrest Champion
                                        spellID == 93821 or             --Gnomergan Champion
                                        spellID == 142234 or    --Brawling Champion (toot toot)
                                        spellID == 142239 or    --Brawling Champion (gorgeous)
                                        spellID == 142242 or    --Brawling Champion (south seas)
                                        spellID == 142243 or    --Brawling Champion (splat)
                                        spellID == 142244 or    --Brawling Champion (bruce)
                                        spellID == 142246 or    --Brawling Champion (rockpaperscissors)
                                        spellID == 142247 or    --Brawling Champion (blingtrong)
                                        spellID == 143625 or    --Brawling Champion (rank 10)
                                        spellID == 93805 or             --Ironforge Champion
                                        spellID == 25780 or     --Righteous Fury
                                        spellID == 17619 or             --Alchemist Stone
                                        spellID == 93795                --Stormwind Champion
                                then
                                        return false
                                else
                                        return true
                                end
                        end
Rather than running a long "if A or B or C or ..." chain you should put all those spell IDs in a table:
Code:
local blacklist = {
     [93795] = true, -- Stormwind Champion
     -- more spells here
}
Then the contents of your filter function can just be:
Code:
     return not blacklist[spellID]
Also:
Code:
BuffBars.customFilter = function(_, _, _, _, _, _, _, _, _, _, _, _, spellID, _, _, _, _, _)
As with assigning values returned by a function, you can just stop the list at the last value you want to catch. You don't need to assign every value. In this case, just stop your list with "spellID" and don't bother naming any of the additional values with underscores:
Code:
BuffBars.customFilter = function(_, _, _, _, _, _, _, _, _, _, _, _, spellID)
----------

Your focus frame is ... I don't even. Why are you creating a bunch of extra frames and listening for events yourself, instead of just using oUF elements and/or tags?

----------

Other than that, your #1 priority should be removing all that duplication.
__________________
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.

Last edited by Phanx : 12-11-14 at 12:01 PM.
  Reply With Quote