Requesting critique on something I've made

[wiMp]

Hey, I am quite new to programming and especially Lua.

I am not sure if this kind of topic is allowed on this forum, or if I've posted in the wrong category, if so, please remove.

I've made myself an addon that tries to detect what equipment set you are wearing and tells you if you're, for example, accidentally wearing you're PvP gear in a PvE instance. I was inspired to write this, since I use the same transmogrification sets for both my PvP and PvE gear and often noticed too late that I was wearing the wrong gear.

I realize that detecting what equipment set you are wearing, is difficult, because you might not have every itemslot filled within an equipment set. So, this will only work if you are using full equipment sets. (Shirt and tabard are ignored.)

Now, I have everything working, but I am just wondering if there's things that I might have missed and should improve on.

TLDR: Not really a question, but a request for critique on the following code.

-- Configuration
local PvPSet    = "enha_pvp"
local PvESet    = "enha"
local msg       = "I think you might have the wrong gear equipped... :("
local point     = "CENTER"
local ofsx      = 0
local ofsy      = 100
local msgfont   = "GameFontHighlight"
-- End of configuration
 
local frame = CreateFrame("Frame")
 
-- Register events
frame:RegisterEvent("PLAYER_ENTERING_WORLD")
frame:RegisterEvent("ZONE_CHANGED_NEW_AREA")
frame:RegisterEvent("PLAYER_EQUIPMENT_CHANGED")
 
-- Create the frame
MyAddon_eqframe = CreateFrame("Frame",nil,UIParent)
MyAddon_eqframe:SetFrameStrata("BACKGROUND")
MyAddon_eqframe:SetWidth(1)
MyAddon_eqframe:SetHeight(1)
MyAddon_eqframe:SetPoint(point,ofsx,ofsy)
 
-- Create the fontstring
local fs = MyAddon_eqframe:CreateFontString(nil, "OVERLAY", msgfont)
fs:SetPoint("CENTER")
fs:SetText(msg)
 
-- Check if the EquipSet and current equipment is the same
function MyAddon_isGearEqSet(equipset)
    -- Put the items from an EquipSet in a table
    if equipset then
        local itemArray = GetEquipmentSetItemIDs(equipset)  
        local itemArrayEqSet = {}
        for i=1,19 do
            if itemArray[i] then
                if i ~= 4 and i ~= 18 and i ~= 19 then -- Ignore shirt, tabard and ranged slot (4 = Shirt, 18 = Ranged and 19 = Tabard)
                    table.insert(itemArrayEqSet,i,itemArray[i])     
                end
            end
        end
        
        -- Put your character's equipment in a table
        local itemArrayPlayerEq = {}
        for i=1,19 do
            if i ~= 4 and i ~= 18 and i ~= 19 then -- Ignore shirt, tabard and ranged slot (4 = Shirt, 18 = Ranged and 19 = Tabard)
                table.insert(itemArrayPlayerEq,i,GetInventoryItemID("player", i))
            end
        end
        
        -- Compare the 
        if #itemArrayEqSet ~= #itemArrayPlayerEq then
            return false
        end
        for i=1,#itemArrayEqSet do
            if itemArrayEqSet[i] ~= itemArrayPlayerEq[i] then
                return false
            end
        end
        return true     
    end
end
 
-- Look for what EquipSet you are wearing
function MyAddon_getGearEqSet()
    local count = GetNumEquipmentSets()
    for i = 1,count do
        local eqSetName = GetEquipmentSetInfo(i)
        if MyAddon_isGearEqSet(eqSetName) then
            return eqSetName
        end
    end
end
 
frame:SetScript("OnEvent", function(self, event, ...)
 
    fs:Hide()
    
    -- Figure out what type of zone you're in
    local _, zoneType = IsInInstance();
    local pvpType = GetZonePVPInfo();
    if zoneType == "arena" or zoneType == "pvp" or pvpType == "combat" then
        if MyAddon_getGearEqSet() == PvPSet then
            fs:Hide()
        else
            fs:Show()
        end
        -- You're in a PvP area, do things here
    elseif zoneType == "party" or zoneType == "raid" then
        if MyAddon_getGearEqSet() == PvESet then
            fs:Hide()
        else
            fs:Show()
        end
        -- You're in a party or raid, do things here
    elseif zoneType == "none" then
        -- You're not in an instance, do things here
    else
        -- You're in some other zone, do things here
    end
    
end)

Thanks!

[Choonstertwo]

I've gone through your code and written up recommendations as I encounter issues:

• Give your visible frame a name (prefix it with your actual AddOn name, not MyAddon) and store the reference returned by CreateFrame in a local variable instead of a global one (when you give a frame a name, it's automatically set to the global variable of that name).
• Make MyAddon_isGearEqSet a local function instead of a global one.
• Don't copy the IDs returned by GetEquipmentSetItemIDs into a new table, just use the one you're given.
• Declare itemArrayPlayerEq once outside of the function and reuse it instead of creating a new table each time.
• Do the same for itemArray, passing it as the second argument to GetEquipmentSetItemIDs
• Don't loop all the way to 19 if you're always going to ignore slots 18 and 19.
• When building the array of the player's current equipment, set the keys directly instead of using table.insert (itemArrayPlayerEq[i] = GetInventoryItemID("player", i))
• Don't use the length operator (#) on arrays with holes in them (the arrays will have nils for any equipment slot without an item)
• Make MyAddon_getGearEqSet a local function.
• Use the return value of GetNumEquipmentSets directly in the for loop counter (this is a very minor issue; what you have now isn't wrong, it's just longer than it needs to be)
• I'm pretty sure you can get rid of your MyAddon_isGearEqSet function by checking the fourth return value of GetEquipmentSetInfo in the MyAddon_getGearEqSet function.
• Another very minor issue: You can use the return value of GetZonePVPInfo directly in the if condition of your OnEvent handler without the extra variable unless you plan to use the value somewhere else in the function.

I've written quite a lot, but many of these are very simple to fix.

This probably belongs in General Authoring Discussion or Lua/XML Help; it'll probably be moved to the appropriate forum.

[wiMp]

Thanks!

You've been very informative.