Code optimization for novices

[Maia]

I've been working on an addon (which atm is only available to my guildies) during the past weeks - and the more it grows, the more I feel the need for optimizing my code.

I never really learned programming, besides writing extensive JavaScript functions for DHTML a few years ago. So I'm not fluent with object orientation, and my lua code is based on the code I see in so many other addons I analyze.

On the other side if I try to grasp the programming guide at lua.org/pil I mostly fail. Partly because I'd need real world exampled, partly because it seems as if it's written for professional programmers.

So if anyone could help me solve some probably pretty basic problems, I'd appreciate it.

1) the 'local function' problem.
I try not to pollute the namespace with global functions and variables - but as soon as my addon has more than one .lua file (and I prefer to have 5-6 smaller files than one with 1500 lines of code) I don't know how to solve that problem.
So how do I define functions that should only be available to my addon but be available in all my .lua files?

2) the 'local variable' problem.
Somewhere I've seen the concept to store all variables in an array, and to set a short alias name to it to have smaller code. E.g.:

myAddonData = {}
local m = myAddonData
m.whatever = "one"
m.somethingelse = true
...

This works great if I don't try to access m.xxx in that single lua file. What doesnt work is to define local m = myAddonData in each of my lua files, as they seem to overwrite each other. So what's the solution (other than removing the 'local', therefor making m accessible globally)?

3) problem with loops pt1:
How do I skip to the next data in my loop? I'd like to replace something like:

for k in whatever do
   if k["one"] then
      if k["one"]["really"] == true then
         doSomething(k)
      end
   end
end

by:

for k in whatever do
   if not k["one"] or not k["one"]["really"] then GOTONEXT end
   doSomething(k)
end

well, what would I have to replace GOTONEXT with to skip all k I don't want to deal with? I hate nested if statements...

4) problem with loops pt2:
The following just doesn't feel right:

function getBestRank(spell,damage)
   local rank = 1
   for i = 1, ranks[spell] do
      if healamount[spell][rank] < damage then rank = rank + 1
      else break
      end
   end
   rank = rank - 1
   if rank < 1 then rank = 1
   return rank
end

If it's not obvious, I'd like to loop through ranks of a healing spell and choose the rank that is slightly below the damage of my target. I'd like to underheal slightly, but not overheal.

5) problem with loops pt3:
Do I really need to declare a local here and then use it? Isn't there a more elegant way?

local really = true
for k in something do
   if not areYouSure(k) then really = false end
end
if really then ... else ... end

6) sorting non-indexed (?) tables/arrays
I'm using an array that stores various info about each raid member which I update on PARTY_MEMBERS_CHANGED:

roster = { }
for i = 1,GetNumRaidMembers() do
   local name = UnitName("raid"..i)
   roster[name] = {}
   roster[name]["raidid"] = 'raid'..i
   _,roster[name]["class"] = UnitClass('raid'..i)
   _,_,roster[name]["group"] = GetRaidRosterInfo(i)
end

Now I'd like to store current health for each unit, and then find the 5 units with the lowest health (yes, the emergency monitor). My solution atm is:

health = { }
for name in roster do
   local unit = roster[name].id
   tinsert(health, { name, UnitHealth(unit)/UnitHealthMax(unit) })
end
table.sort(
   health, 
   function(a,b)
      return a[2] < b[2]
   end
)
needy = { }
local i = 1
for k,v in health do
   needy[i] = {}
   needy[i].name = v[1]
   needy[i].hp = v[2]
end

But, do I really need to define a second and a third array? I guess not, but I don't know how to sort the array 'roster' by e.g. roster[name].hp and then access the first five...

I know. Lots of questions. Some may sound very noobish to you. But if you could help optimizing the one or other code snippet, I'd really appreciate it. I'm trying to learn something new from the lua.org guide day for day, but I havent found an answer to my question yet - other than "yeah, use metatables and __.index" - "uum?".

Thanks in advance.

PS: I just found out about the existance of pastbin.com, so I uploaded the sample code snippets there. So if you have a solution, you can update the code online:
http://maia.pastebin.com/656331

[elviso]

Hello

I am quite the LUA beginner as well, but I've learned alot lately so I'll try to help if I can....but no guarantees here

1) the 'local function' problem.

I really don't know on this one, having never tried using >1 .lua file, I was just under the impression that all the files listed in your .toc would be considered as a whole. For instance, I know I can call functions in my .xml that are defined in my .lua, however I haven't tried .lua -> .lua. I'm eager to hear what the solution to this is myself.

2) the 'local variable' problem.

See above.

3) problem with loops pt1:

I made all possible-but-untested-and-probably-grossly-wrong changes here:

http://maia.pastebin.com/657356

4) problem with loops pt2:

See pastebin.

5) problem with loops pt3:

See pastebin.

6) sorting non-indexed (?) tables/arrays

I have no freaking clue

I'm almost certain that what I gave is certaintly not the best method to do things, but I figured I'd at least give my input and hopefully at least 1 part of it will be useful for you. Good luck!

[MoonWolf]

Okay let me give a few pointers yust on point one.
1
variables in lua are nothing but references... a function is nothing more then a reference to a function with a name.

a table can contain any datatype.....

Therefor you can store Functions in a table. Like this.

Myaddon = {}
Function Myaddon:Myfunction(text)
return text
end

If all your function are placed in the table you keep the global namespace clean. The addon object does not have to be local so it is available to new .lua files. You can ofcourse store other data types in here perfectly fine too.

Myaddon.shouldidosomething = true

[svip]

I have to confess I've been away from WoW for a long time but such eagerness to learn is a rarity so had to reply. My answers may be off a little though with the absence (Lua is a weird creature).

1) the 'local function' problem.
MoonWolf answered this well.

2) the 'local variable' problem.
I did something like this. I think your problem may be that the 'local m =' assignments are happening before myAddonData is set in some files. Make sure you create myAddonData in the first file listed in your toc and hopefully the 'local m = myAddonData' will work in all files.

3) problem with loops pt1:
Though the reply above is quite creative, the 'continue' keyword is the direct answer.

4) problem with loops pt2:
Is this what you're after?

function getBestRank(spell,damage)
   local i;
   // could arrange the data better here but that's really being anal
   for rank = 1, ranks[spell] do
      if healamount[spell][rank] > damage then break
   end
   if i > rank then rank--
   return rank
end

Alternatively you could subtract a number (say, 200) from damage before the loop and then use the rank found, to keep it within a safety margin.

5) problem with loops pt3:

local really = true
for k in something do
   if not areYouSure(k) then really = false end
end
if really then ... else ... end

I'm not really sure what the intention is here. But if you need to perform an action only in something happens in the loop then yes, that is one way - another would be performing the action inside the loop on the match and returning/breaking out afterwards. Or in this case, the reverse - if false, just return right there.

6) sorting non-indexed (?) tables/arrays
Personally I'd advise against your table of everyone in the raid - the WoW client already has this information and duplicating it in your own table is really a waste of space. If you need this table (and plenty of reasons for that) I suggest ditching the name/class from it - unless you need to look up by name/class really often - as in several times per second.

Here's some pretty simple (and untested) emergency monitor update code - though you can modify it to use your table which really should be indexed by raid id. Note that you shouldn't run this code every frame (as in every update()) - people don't react that fast anyway and it'll contribute to choppiness.

I think I see a bug in your code by the way - in theory it'll only work if there aren't 'spaces' in the raid, for example you move one person to the last slot in last group but noone in the previous groups. Though if GetRaidNumMembers() returns the max 'slot' used that's untrue. I haven't worked much with raid code though so don't take my word for this. But if I'm right, just scan for 40 as the max number. That and this code snippet can easily be improved for smaller raids though.

function updateEmergencyMonitor()
    local temp = {}
    for i = 1, 40 do
        temp[i].id = i;
        if UnitHealth('raid') then
            temp[i].pct = UnitHealth('raid' .. i) / UnitHealthMax('raid' .. i)
        else temp[i].pct = 1; end // Make sure they don't get sorted low if not there
    end
    table.sort(temp, function(a,b) return a.pct < b.pct; end)
    // Now temp[1] will be the lowest % member, etc
end

If you have further questions that seem 'noobish' you're welcome to PM me for a faster reply, I don't have time for the actual coding anymore but I do enjoy the problem solving.

[Maia]

Elviso, MoonWolf, Svip, thanks a lot (plus Mark and Tyrone who submitted suggestions directly in the pastebin).

As for the local variable problem:
Svip, I did declare myAddon={} in the first lua file listed in the .xml and then declare m=myAddon in all lua files (and in the main file obviously after the myAddon={}) - it doesn't work though. Guess using such a short alias won't work as I hoped.

As for "problems with loops, pt1":
My goal is to have code that is easy to read. I prefer short code, short lines, and not a ton of nested if statements. The hint regarding 'continue' seems interesting, but I can't find any reference to it on lua.org - sure it does exist in lua and not only C? As I understand it, a continue will jump to the next iteration of a loop, right? (this is what I'd be looking for)

problem with loops, pt2 (the underhealing function):
Svip, I don't understand your code: you're declaring "local i;" and then comparing rank to i - but i never is a number, is it? But Elviso's suggestion seems to be an easy way:

function getBestRank(spell,damage)
   local rank = 1
   for i = 1, ranks[spell] do
      if not healamount[spell][i] < damage then break end
      rank = i  -- use i to assign the rank after the break to keep from over-ranking
   end
   return rank
end

problem with loops, pt3: my intention is listed here: http://maia.pastebin.com/656808 (line 129-147).

sorting tables:
Hmm. I've been indexing by id and not name for a while, until Cladhaire mentioned on the wowace forums that it's better to index by name (one reason was that it's then easily possible to use the same functions for party members, pets,... but I think another reason was that a raidid can change at any time, whereas the name always stays the same) - which is why I changed it. And I guess I do need the table, as I do need the class info several times I second (I also need info if someone has aggro or not, see above, or if someone is being healed by others).

I guess I'll have to explain my goal: I'm trying to write a small emergency monitor which won't only sort by health, but add various modifiers. So e.g. a tank who has aggro, is not being healed by others, who is in your subgroup and has 60%hp has priority over a rogue in a different subgroup, being healed by another healer and has 50%hp. Which is why I'd want to gather all that info every 0.2sec and then display a list of the 5 people who you should be healing most.

(oh, and yes, I could live without that addon - but at the moment I'm having more fun writing it than spending even more time raiding or in a battleground, and learning programming is pretty nice as well).

So I could use the following code:

function updateEmergencyMonitor()
   local temp = {}
   for i = 1,40 do
      temp[i].id = i
      local hp = getHealthPct(i) --0-100
      local aggro = getAggro(i) --returns -20 if aggro, otherwise 0
      local healedbyothers = getHealData(i) --returns +10 if healed by others, else 0
      local classmodifier = getClassModifier(roster[i].class) --returns -20 for a warrior, +10 for a rogue,...
      temp[i].priority = hp + aggro + healedbyothers + classmodifier
   end
   table.sort(temp, function(a,b) return a.priority < b.priority end)
end

Well, that's probably the easy part. If I really want to keep my roster[name] (and lets say its only for theoretical reasons), I'm still somewhat clueless how to sort that. Probably if I'd understand how to use this: http://www.lua.org/pil/19.3.html I'd know how

Thanks again.

[svip]

Originally Posted by Maia As for "problems with loops, pt1":

You got me - spent the last several months immersed in C++; It was hard enough for me not to end every statement with a semicolon even if it's optional in Lua;
Lua is, shall we say, heavily influenced by C but not in this matter I guess. But preferring short code is something to be proud of - stick with that. So is there any way you can rewrite this? For example putting the for loop in another function, or the fuctionality if the if check is true.

Originally Posted by Maia problem with loops, pt2 (the underhealing function):

Sorry for the weird code - I replaced i with rank before posting it and I guess I did a pretty bad job of it.

function getBestRank(spell,damage)
   for rank = 1, ranks[spell] do
      if healamount[spell][rank] > damage then break
   end
   if rank > 1 then rank--
   return rank
end

Originally Posted by Maia problem with loops, pt3: my intention is listed here:

Clever. Wrote a target-of-target thing when the functionality was first announced (ex-MT warrior here) and that bit annoyed me. See my pt1 answer though.

A minor change to make it simpler if you stick with this would be

   roster[name]["aggro"] = aggro

Instead of the if statement at the end.

Originally Posted by Maia sorting tables: ...

That's one beautiful beast of an idea - almost brings a tear to my eye.

Now, I think the array is in an arbitrary order when there's no index in it. If I'm wrong about that there should be some clever way to do it with the original array but I'm at a loss here. So instead I'll have a look at making your code simpler.

First of all, table.sort. table.sort only works to ordered an indexed table, as you've found, so we will need the second array.

local emerg = {}

for r in pairs(roster) do // For everyone in the roster
    // Put the priority code in another function. Pretty!
    // Also stores it directly in the roster. If it's going
    // to be used so often we may as well..
    r.priority = getHealPriority(r);
    table.insert(emerg, r) // Store a reference to the roster entry
end

// Sort it
table.sort(emerg, function(a, b) return a.priority < b.priority);

// Theoretical code from a warped mind - but this should discard
// the superfluous tablebits
while emerg[5] do table.remove(emerg, 5) end

// I assume you stored the id in the roster so the following should
// get the name of the first person on the list
local firstPerson = UnitName('raid' .. emerg[1].id)
// And of course -
// roster[UnitName('raid' .. emerg[1].id)] == emerg[1]

[Maia]

Originally Posted by svip So is there any way you can rewrite this? For example putting the for loop in another function, or the fuctionality if the if check is true.

Well, this might make it a bit easier, but still not perfect:

function hasAggro(unit)
   if UnitExists(unit.."target") and UnitCanAttack("player",unit.."target") and UnitExists(unit.."targettarget") and UnitInRaid(unit.."targettarget") and UnitAffectingCombat(unit.."targettarget") then return true
   else return false
   end
end
for name in roster do
   if hasAggro("raid"..roster[name].id) then roster[name].aggro = true
end

I'd prefer something like:

for name in roster do
   local unit = "raid"..roster[name].id
   if not UnitExists(unit) then continue end
   if not UnitCanAttack("player",unit.."target") then continue end
   if not UnitExists(unit.."targettarget") then continue end
   if not UnitInRaid(unit.."targettarget") then continue end
   if not UnitAffectingCombat(unit.."targettarget") then continue end
   roster[UnitName(unit.."targettarget")].aggro = true
end

Originally Posted by svip So instead I'll have a look at making your code simpler.

Thanks a lot! Two things are confusing me - your last line and using r.priority instead of roster[r].priority in the loop. Your version sounds simpler, but I didn't assume r was an object (?) in my "for r in roster". (I guess I need to work on my programming vocabulary).
So the following should work?

local emerg = {}
for name in pairs(roster) do
   name.priority = getHealPriority(name)
   -- so this is the same as roster[name].priority = getHealPriority(name) ?
   table.insert(emerg, name)
end
table.sort(emerg, function(a, b) return a.priority < b.priority)
while emerg[5] do table.remove(emerg, 5) end
-- now you're saying:
-- roster[UnitName('raid' .. emerg[1].id)] == emerg[1]
-- so emerg[1] is the same object as roster[emerg[1]]?
-- if yes, then:
for i in emerg do
   frame[i].name:SetText(emerg[i])  -- UnitName
   frame[i].name:SetColor(getRaidColor(emerg[i].class))  -- color depending on class
   frame[i].SetValue(emerg[1].hppcct)  -- set bar, min/max values 0,100
end

Finally some progress! I've been working on the various other functions for so long but never realized how to display the info I'm gathering. Until today I was gathering all that data on each spellcast to make sure the addon doesn't lag (to then dump the people in range into the chat window), but the more I streamline the code, the more I dare to run all these functions every 0.2sec and update frames.

Next step: find out how to dynamically create frames. I'd like to have an addon without xml file

[svip]

Originally Posted by Maia I'd prefer something like: ...

I guess you could do this:

for name in roster do
   local unit = "raid"..roster[name].id
   if UnitExists(unit)
       and UnitCanAttack("player",unit.."target")
       and UnitExists(unit.."targettarget")
       and UnitInRaid(unit.."targettarget")
       and UnitAffectingCombat(unit.."targettarget") then
           roster[UnitName(unit.."targettarget")].aggro = true
    end
end

By the way, in most modern programming languages - Lua included for all I know - processing of such an if statement would halt at first 'false' it encountered so it's quite efficient.

Originally Posted by Maia Thanks a lot! Two things are confusing me - your last line and using r.priority instead of roster[r].priority in the loop. Your version sounds simpler, but I didn't assume r was an object (?) in my "for r in roster". (I guess I need to work on my programming vocabulary).

Here's the dealio: All variable names in Lua are simply references to the data (objects really) they point to. So you can set, like above, a variable to an array element and treat it as if it was the original element (it is, they both refer to the same date).

For example when I picked out an array element to do lots of work with, I'd do somehthing like:

      local r = roster[UnitName('targettarget')];
      if r.class = 'Rogue' then do stuff with r;

IMO, when you start thinking of them as references your Lua efficiency increases dramatically.

Originally Posted by Maia Until today I was gathering all that data on each spellcast to make sure the addon doesn't lag, but the more I streamline the code, the more I dare to run all these functions every 0.2sec...

Wish more programmers thought like that.

[SlackerJer]

I think I saw it posted above in some form, but here's the standard function I use when I need to iterate a table that's sorted by it's keys. You can use it in place of your normal pairs() function if you need it. Note that it does create another table to sort. So it's probably not something you'd want to run every .1 seconds on a large table.

function PairsByKeys(t, f)
	-- t = table
	-- f = optional sort function
	local a = {}
	for n in pairs(t) do table.insert(a, n) end
	table.sort(a, f)
	local i = 0	  -- iterator variable
	local iter = function ()   -- iterator function
		i = i + 1
		if a[i] == nil then return nil
		else return a[i], t[a[i]]
		end
	end
	return iter
end

[Maia]

Thanks everyone for your help. I really appreciate it!

[Miravlix]

Local is handled at load time, the second it's executed it takes effect and is availble to the file/function/if/for/etc scope.

file1.lua

-- Create a local table
local m = {}

-- A variable
m.debug = true

-- A function
m.foo = function() if m.debug then print debug end end

-- Make a global referance
MyAddon = m