12-28-14, 12:23 PM | #1 |
Which is Better?
Which code is written in a more correct way or are they both the same?
Style A: Lua Code:
Style B: Lua Code:
Thanks for any opinion on this. Coke |
|
12-28-14, 01:31 PM | #2 |
My guess: as you're unregistering ADDON_LOADED without checking the addons name both Versions will only work if there are no other addons enabled or if there are no other addons loaded before your addon.
[e] Apart from such bugs imho the second one is just an awkward overcomplicated/redundant way to do things. Last edited by Duugu : 12-28-14 at 01:33 PM. |
|
12-28-14, 04:37 PM | #3 | |
It's pointless tho if your addon's name is at the end of the alphabet. I personally like the second one, since it's gonna help you to keep organized with more complex projects, specially if you want to call functions from different files. But for such a small ones it's pretty much up to you which one you prefer, since the resourse usage difference is negligible even if you duplicate thoose functions. Last edited by Resike : 12-28-14 at 04:45 PM. |
||
12-28-14, 04:47 PM | #4 |
12-28-14, 04:48 PM | #5 |
12-28-14, 04:50 PM | #6 |
If I remember correctly the event fires for every addon that is loaded. I thought that's why arg1 provides the addon name.
|
|
12-28-14, 05:23 PM | #7 |
correct, all he needs is the arg1 == check, not the event check.
if arg1 == "cBuffs" then instead of if event == "ADDON_LOADED" and arg1 == "cBuffs" then |
|
12-28-14, 06:18 PM | #8 |
12-28-14, 06:56 PM | #9 |
I'm starting to think we are speaking about two different things. I was merely adding (regardless of the code below it) that there is no need for the event check, there's only one registered, it can only fire for addon_loaded.
I believe what you are getting at, is the fact that, as currently written, this script would fire as soon as the first addon is loaded, which I'm confident is not cbuffs. Since the initial if check requirements would not be met, the script would skip to the slashcommand creation, and would then unregister the only event it's listening for. This would essentially ensure the script never fires when he wants it to. If that's what you're saying, I agree, and in no way intended to imply or convey otherwise. I just noticed a relatively common, and small, unoptimization. If that's not what you're saying, please expand, because I don't see where I am incorrect in my suggestion. |
|
12-28-14, 07:26 PM | #10 |
12-28-14, 07:28 PM | #11 |
12-29-14, 08:59 PM | #12 |
There is no value in delaying any of this stuff. You can just do all your function hooking in the main chunk, since all of the functions you're hooking are defined in FrameXML and not in LoD portions of the UI. You can also just call SetScale on the BuffFrame in the main chunk, since the default UI never calls that method anywhere. Just get rid of the whole frame and event handler.
However, for future reference if you do have things that need to be delayed, I'd just use the first style. Factoring out each item into its own function is wasteful. As a general rule, if a function will only ever be called from one place (either one time or 1000 times) you should just take the code from that function, and move it to the place where you call the function instead. Otherwise you're just wasting CPU cycles by adding another function call. The only reason to factor setup things out into separate functions would be if you were going to make each item toggle-able on the fly with in-game options, but hooksecurefunc isn't togglable -- once it's done, it can't be undone -- so you would need to check the setting inside the hook function, and there still wouldn't be any point in having a separate function that does nothing but install the hook. Overwriting SecondsToTimeAbbrev and setting scales, however, are undoable, so you could toggle those on the fly, so if you went that way, it would be sensible to factor those out into a function that either did or undid those things, eg. Code:
local old_SecondsToTimeAbbrev = SecondsToTimeAbbrev local new_SecondsToTimeAbbrev = function(seconds) -- your code here end function MyAddon:EnableTimeHook(enable) if enable then SecondsToTimeAbbrev = new_SecondsToTimeAbbrev else SecondsToTimeAbbrev = old_SecondsToTimeAbbrev end end Code:
local origSecondsToTimeAbbrev = _G.SecondsToTimeAbbrev local function SecondsToTimeAbbrevHook(seconds) origSecondsToTimeAbbrev(seconds) local tempTime if (seconds >= 86400) then tempTime = ceil(seconds / 86400) return '|cffffffff%dd|r', tempTime end if (seconds >= 3600) then tempTime = ceil(seconds / 3600) return '|cffffffff%dh|r', tempTime end if (seconds >= 60) then tempTime = ceil(seconds / 60) return '|cffffffff%dm|r', tempTime end return '|cffffffff%d|r', seconds end SecondsToTimeAbbrev = SecondsToTimeAbbrevHook 2. There is no reason to assign a value to your tempTime value when you just return that value on the very next line. Just return the value directly and skip creating a variable: Code:
if (seconds >= 86400) then return '|cffffffff%dd|r', ceil(seconds / 86400) end Code:
function SecondsToTimeAbbrev(seconds) -- your code here end Code:
hooksecurefunc('AuraButton_Update', function(self, index) Code:
if (self:match('Debuff')) then duration:SetFont(font, 12, 'THINOUTLINE') else duration:SetFont(font, 12, 'THINOUTLINE') end 2. This is a good example of why naming a string value self is confusing. My first thought when I saw this part was that your code should be raising an "attempt to call match (a nil value)" error, and this will probably be the first throught of most experienced Lua programmers who look at this code. It will also be confusing to you if don't look at this code for a few weeks, months or even years, and then come back to it to make a change in the future. 3. String operations (match, find, format, etc.) are fairly expensive and should be avoided when possible. In this case, your goal is to distinguish between buff icons and debuff icons, so you can achieve that more efficiently by checking for to see if the buffon has a symbol member -- debuff icons have it, but buff icons don't. Code:
if button.symbol then -- it's a debuff icon else -- it's a buff icon end Lua Code:
First, not really a problem, but you don't need to check if event == "ADDON_LOADED". Your frame is not registered for any other events, so it will never receive any other events. Every event it receives will be an ADDON_LOADED event. Second, and more importantly, the "some other stuff" and the UnregisterEvent lines are called on any ADDON_LOADED event, not just the one where arg1 == "cBuffs", so you're adding a slash command and then unregistering your event on the first event received. That's probably your addon's event, but it's not guaranteed to be, and if it's not, your frame will never receive your addon's event because you already told it to stop listening. You need to either (a) wrap the entire contents of the event handler in the check: Code:
if arg1 == "cBuffs" then -- some stuff was here -- some other stuff was here self:UnregisterEvent("ADDON_LOADED") end Code:
if arg1 ~= "cBuffs" then return end -- some stuff was here -- some other stuff was here self:UnregisterEvent("ADDON_LOADED")
__________________
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. |
|
12-30-14, 06:03 AM | #13 |
Just my 2 cents: I think it is a good practice to always check the event even if you just have one registered, always doing this will make maintaining your code easier. You will just need to add lines, same about event handler, sometimes you don't need to use the parameters and could just
f:SetScript("OnEvent", function() but if you always declare f:SetScript("OnEvent", function(self, event, ...) you made your code more generic and easier to maintain. Scripted language main point is to descriptive, being a performance freak in scripted languages does you no good when that makes your code harder to read and maintain. Of course that is no excuse to poor coding though. Last edited by Banknorris : 12-30-14 at 01:42 PM. |
|
12-30-14, 06:29 AM | #14 |
If I'm writing a simple "delay for ADDON_LOADED or PLAYER_LOGIN" frame that only handles that one event by design, I would never add an event check, since it's blatantly obvious that the frame is only handling one event and will only ever handle one event. Adding an event check in this type of code just wastes (imaginary) CPU time and adds useless clutter. I'd argue that the clutter aspect actually makes your code harder to maintain, since when you come back and look at it and see an event check, you assume it must be able to handle other events, and then are confused as to why the hell you included an event check on a frame that only handles one 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. |
|
12-30-14, 11:50 AM | #15 | |
lua Code:
lua Code:
Last edited by ravagernl : 12-30-14 at 12:36 PM. Reason: Oh Brain you... (bonk bonk bonk) |
||
12-30-14, 11:59 AM | #16 | |
Well, that's not quite relevant to what I was saying; I was talking about a frame that's created for the sole purpose of handling a single hardcoded event, typically just one time, such as delaying something for after saved variables are loaded or after spell information is available. Obviously if your frame is handling more than one event, or if you dynamically register and unregister events, you probably have to check the event, and you are doing that in the code you posted.
Also, why go with "KillEvent" instead of just sticking to convention and naming that "UnregisterEvent" ?
__________________
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. |
||
12-30-14, 12:21 PM | #17 | |||
It indeed is, wrote that from memory and didn't test it, I'm a bit tired and chilling on my laptop
Last edited by ravagernl : 12-30-14 at 12:34 PM. Reason: damn you bbcode! |
||||
12-30-14, 07:30 PM | #18 |
Thank you everyone for the advice and help.
So here is what I have came up withy for the cBuffs.lua Lua Code:
Again thank you for the help. Coke |
|
12-30-14, 07:51 PM | #19 |
Much better, though you shouldn't need the "if (button) then ... end" wrapper around the contents of your AuraButton_Update hook, since the default UI should never be calling update functions for buttons that don't exist. Leaving it won't really affect performance, it just adds an extra level of indentation for no particular reason. Smaller amounts are usually better when it comes to code.
__________________
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. |
|
12-31-14, 08:56 PM | #20 |
Thank You Phanx I removed the "if button ... end"
Finally almost complete with my personal UI Still to do: A) Find/work out a slim downed (with only the option I want) chat mod like PhanXChat or nChat from NeavUI. B) Get nData to work better with WoD and use less resources. Thanks Everyone for your advice and help. Coke [EDIT] for those that are wondering what this buff mod does it pretty basic It scales the buffs bigger and mover the text inside the buff icon (see image below) Last edited by cokedrivers : 12-31-14 at 09:12 PM. |
|
WoWInterface » Developer Discussions » Lua/XML Help » Which is Better? |
«
Previous Thread
|
Next Thread
»
|
Display Modes |
Linear Mode |
Switch to Hybrid Mode |
Switch to Threaded Mode |
|
|