Misc issues with an addon

[Duugu]

Originally Posted by Phanx First and foremost, stop doing this: for _, event in next, { "UNIT_COMBO_POINTS", "UNIT_POWER", } do caelUI.powersound:RegisterUnitEvent(event, "player") end There is absolutely no benefit to registering events this way. You're just wasting memory on a table and CPU on a loop. If you were dynamically altering the list of events to register this might make sense, but for a fixed list of 2 events, just do it normally: caelUI.powersound:RegisterUnitEvent("UNIT_COMBO_POINTS", "player") caelUI.powersound:RegisterUnitEvent("UNIT_POWER", "player")

Come on. It happens once if the addon loads. And it is easy to maintain.

[Phanx]

Originally Posted by Duugu Come on. It happens once if the addon loads. And it is easy to maintain.

"It only runs once" isn't really a valid excuse for writing unnecessarily inefficient and overly verbose code. There absolutely no benefit to writing it that way, and even if you want to write your own code that way "just because", posting it on forums as an example of working code just means that other people are likely to copy and paste and use it in their own addon without understanding that's pointlessly overcomplicated.

Originally Posted by Rainrider You also don't have a local reference to MAX_COMBO_POINTS. You don't check for vehicle combo points either (there was a daily rep quest back in WotLK that used that, maybe there are others too). You could also set MAX_POWER to 4 for warlocks as long you are not using this for demonology and spare a function call in that case.

The lack of upvalues for constants was intentional, but if you wanted to go that route, you could just set MAX_POWER for every class (registering additional events for those that require them) and never call UnitPowerMax in the main code path at all.

Also, if you wanted to support vehicles, you could just include them in the event registration:

caelUI.powersound:RegisterUnitEvent("UNIT_COMBO_POINTS", "player", "vehicle")
caelUI.powersound:RegisterUnitEvent("UNIT_POWER", "player")

[Duugu]

Originally Posted by Phanx "It only runs once" isn't really a valid excuse for writing unnecessarily inefficient and overly verbose code. There absolutely no benefit to writing it that way, and even if you want to write your own code that way "just because", posting it on forums as an example of working code just means that other people are likely to copy and paste and use it in their own addon without understanding that's pointlessly overcomplicated.

Imo it is a valid excuse.
There are benefits. It's pretty easy to add new events to the list. It's easy to move the event list makeup or the list itself to somewhere else later on, it's easy to overview the event list, etc.
Not compelling and a matter of taste, but why not?

Even if there are no benefits there are also no drawbacks. In the current scenario the effect on memory and CPU performance is so incredibly low that it is immeasurable.

I would agree if you would say "It is inefficient. It doesn't really matter at this point but don't do it in you OnUpdate every 2 ms."
But saying "stop doing this" in a general way - if it matters or not - is imo insiting on rules without respecting the actual situation and sounds kind of wrong-headed to me.

[Phanx]

Originally Posted by Duugu There are benefits. It's pretty easy to add new events to the list. It's easy to move the event list makeup or the list itself to somewhere else later on, it's easy to overview the event list, etc.

It's just as easy to add new events, move the list, or view the list if you just write code that does what you want to do directly instead of an unnecessary loop over an unnecessary table.

[Seerah]

I am a teacher, and I agree with Phanx here on this point:

even if you want to write your own code that way "just because", posting it on forums as an example of working code just means that other people are likely to copy and paste and use it in their own addon without understanding that's pointlessly overcomplicated.