Originally Posted by Phanx
Even setting aside the conceptual flaws (your code is way more complicated than it needs to be to do what it does) there are several basic issues with your code that make it pretty unsuitable as an example of how anything should be done.
This creates a global variable named "Tooltip". You say you know this, yet you did it anyway. This is 100% bad and you should never do it for any reason. Doing it in something you're calling an example for others to use is even worse than doing it in code for yourself.
Lua Code:
Tooltip._tooltip = 'Tooltip._tooltip'; Tooltip._tooltip_frame = CreateFrame('GameTooltip', Tooltip._tooltip, nil, 'GameTooltipTemplate');
This creates a global variable named "Tooltip._tooltip" which, though slightly less bad than a global "Tooltip", is still not good. Global variables should always follow two simple rules:
1. It should be unique, so there is no chance of it colliding with another badly named or accidentally leaked global. At best, global variable collision will make an addon break. At worst, it will break the entire UI and make the game unplayable.
2. It should clearly identify the origin (ie. what addon created it) and purpose of the value it contains, so if it shows up in an error message, stack trace, /framestack tooltip, or other debugging context, there is some hope of the user figuring out where it's coming from. |
I though that my note about tainted global is clear enough.
It's just for
clarity:
Originally Posted by Benio
note that the following util uses global Tooltip, but it's just for clarity
|
If you wish implementing this util in your addon, then sure, you should follow all those tainting rules.
Originally Posted by Phanx
Lua Code:
Tooltip.GetTooltip = Tooltip.GetTooltip or function(itemLink)
Since this code is running in the main chunk of your addon, it's only ever executed once. There will never already be a "GetTooltip" method on your "Tooltip" object, so there's no point in checking for one before defining it.
Lua Code:
Tooltip.GetSpellDuration = Tooltip.GetSpellDuration or function(itemNameORID)
Lua Code:
Tooltip.GetUnitAuraPandemic = Tooltip.GetUnitAuraPandemic or function(unitID, itemName, rank, filter)
Same as above. No reason to check for things that you already know don't exist. |
It's util. Util (or utility) is indented to be used multiply times.
This means, that it is highly propable that a few addons will use sam utility.
Then we can just use already existing one.
Originally Posted by Phanx
Lua Code:
if not text then break end;
While this won't break anything in the context of scanning an aura tooltip, it's also not necessary in that context -- if tooltip:NumLines() is 3, then the tooltip has 3 lines with text; you don't need to verify that there's text -- and will break scanning in other types of tooltips, since item tooltips, for example, can have empty lines in the middle of them. |
Optimalization. This util is indented to be used to retrieve spell info. No spell I am working with in my indev addon have empty line before mentioned duration. They have empty line after it and thus, it's worth skipping this.
Originally Posted by Phanx
Writing this at the end of a function does absolutely nothing, and there's no reason to write it. It just takes up space for no reason. |
Clarity. Some people likes to see what function returns. However you're absolutelly free to remove it, if you only wish.