Authors CurseForge

Arrowmaster, things like this are useful for debugging weird shit IMO, i've had several instances where this sort of thing would have been useful to find out why a function isn't working properly or where things are in following a "what is self in this context" problem. Tho i got by with /dump, this would have been prefered.

How about making tabcompletion work on tables

/print Prat.<tab through fields>

That'd be spiffy.

Quote from Wintrow2

Arrowmaster, I don't understand this sentence. But I can tell you this, as OrionShock mentioned, this is a developers tool (which is why it's in the "Development Tools" category.

It lets the developer inspect a variable without awkward slash commands.

I find it hard to believe ANY developer NOT wanting this library (still, it could happen :p).

While I am not opposed to developer tools, I personally find this library unneeded for any of the 12 or so addons in my development. /print and /run are more than enough sufficient tools for me.

1. You should not use a library for table recycling. This has been done in the past (See CompostLib), and has proven disastrous for addon debugging. What happens is that AddOnA uses a table from the library, later releases the table back into the library, but leaves dangling references to it. AddOnB then uses this recycled table, and experiences errors that AddOnA is causing by using the dangling referenec to the table (such as adding/removing entries to the table with tinsert).

This is impossible to debug because the table's origins would be totally unknown, addonB's code is perfect, but A isn't, but because of A, B is having errors. It's been tried, used, and proven to be a disaster. Remove table reycling from LibHandyHelper. Each addon should use its own local table recycling to contain possible coding errors.

We're talking safe coding practices here.


2. You have ugly code. Consider:

    -- The table has an inline group that contains controls for doing stuff on the variable
    local sg = hp:new();
        sg.name = sname;
        sg.type = 'group';
        sg.order = 1;
        sg.inline = true;
        sg.args = hp:new();
    n.args[s] = sg;
        
    local label = hp:new();
        label.name = 'String variable';
        label.type = 'description';
        label.order = 1;
    sg.args.label = label;

Why not just do

local sg = {
  name = sname,
  type = "group",
  order = 1,
  inline = true,
  args = {
    [s] = sg,
    label = {
      name = "String Variable",
      type = "description",
      order = 1,
    },
  },
}

It's faster (to execute), cleaner to see, and if you really want to, you can still release these tables back to the recycling table pool later. As it is, your addon has zero intention of recycling these created nodes because you don't know if the calling addon will release it properly without dangling references.

And "s" is a table in your code, using tables as a key in the args table is disallowed by AceOptions table standards because it has to be parsable and printable by other AceOptions implementations such as AceConsole.

3.

-- what's this for anyway? isn't mr.embeds empty at this time??
for addon in pairs(mr.embeds) do
    mr:Embed(addon)
end

You wrote a library without knowing what this does? It takes the existing addons that already embedded the library, and reembed them in case of library upgrades (newer version of the library in a second addon), by overwriting the old function references with the new references in the upgraded library.

4. I don't even want to comment on LibHandyHelper, it has so many useless functions that are 1-line function wrappers, if any calling addon code needed the functionality of say "UnitIsPartyPet" or "GetRaidPetOwnerID", they would have just called the actual functions directly, and skip the overhead of your 1-line function wrappers.

We call this the Alar syndrome (See the AlarLib, for being a library that contains 200 wrappers adding unnecessary overhead).

Libraries, being shared code, are primarily designed with efficiency and speed in mind, the function wrappers are against this philosophy. They might be handy to use, yes, but they are inefficient to use.

Particularly because your library doesn't even upvalue commonly accessed functions such as type, next, pairs by doing

local type, next, pairs = type, next, pairs

at the start of the file, to avoid global function lookups.

5.

do
    local pool = setmetatable({},{__mode='k'})
    hp.new = function()
        local t = next(pool)
        if t then
            pool[t] = nil
            return t
        else
            return {}
        end
    end
    hp.del = function(t)
        for k in pairs(t) do
            t[k] = nil
        end    
        pool[t] = true
    end
end

You copied table recycling code, without considering the fact that you also need to setmetatable(t, nil) on a recycled table in the hp.del() function, or existing metatables in recycled tables will haunt you forever when another addon tries to use the table.

AceGUIWidget-TreeGroup didn't need to nil out metatables because it knows its own code doesn't set metatables on its tables, and nobody else outside the file can use the recycling functions.

Also,

for k in pairs(t) do
    t[k] = nil
end

Can just be called as wipe(t) now in wow's lua, which is a lot faster.

6.

-- Version of table.getn that gets a count of ALL members
hp._CountMembers = function(Table)

    local cnt = 0;
    local k, v = next(Table);
    while k ~= nil do
        -- v is probably never nil in this case, but just to be sure...
        if v ~= nil then
            cnt = cnt + 1;
        end
        k, v = next(Table,k);
    end
    return cnt;

end

Are you aware that pairs() is a lot faster than next() ?
And that v cannot be nil so the check is redundant?

-- Version of table.getn that gets a count of ALL members
hp._CountMembers = function(Table)
    local cnt = 0;
    for k, v in pairs(Table) do
            cnt = cnt + 1;
    end
    return cnt;
end

7.

-- Like the isnull function in SQL, returns a substitute value if first value is nil
hp._isnil = function(value, repl)
    if value == nil then
        return repl;
    else
        return value;
    end
end

I wanted to choke on this one, because
n = isnil(a, b)
would be identical to
n = a == nil and b or a
assuming b is not the value false or nil, which in 99.99% of use cases, b won't be either of them.


---------
There, you wanted critique, I gave it. Summary:

- LibMindReader could be useful, but the code is ugly, and the use of recycled tables will bite you at the end of the day if some other addon developer fails to destroy dangling references.

- LibHandyHelper is primarily useless. While we welcome any developer to join the wowace community, we do want the stuff we host to be good code, and if it isn't good code, you'll hear no end of it. I only looked in it for about 3 minutes, If I tried harder, I'll find more issues. The code shows little understanding of Lua intrinsics, and short circuit evaluation.

Quote from Wintrow2

Follow Up Question: If I loop through a table w pairs() and I delete (set to nil) a member will the iterator mess up or properly hand me the next member as if I hadn't deleted anything?

You may safely delete entries that have already been iterated over (plus the current key being iterated on) when using pairs(). However, if you delete any key that hasn't been iterated over yet, or add a new key, you will get undefined behavior, and the code will choke/crash.

Quote from Xinhuan

Also,

for k in pairs(t) do
    t[k] = nil
end

Can just be called as wipe(t) now in wow's lua, which is a lot faster.

Has wipe(t) been introduced in 3.0 ? I didn't notice it in the UI concise change post.

Quote from Adirelle

Has wipe(t) been introduced in 3.0 ? I didn't notice it in the UI concise change post.

Yes. It is added in 3.0, usable in both secure header code and non-secure code. wipe(t) will completely wipe out the contents of a table non-recursively (remove all key-value pairs). Any attached metatable is not affected.