Quantcast Requesting critique on something I've made - WoWInterface
Thread Tools Display Modes
01-27-14, 01:13 PM   #1
A Deviate Faerie Dragon
Join Date: Mar 2008
Posts: 10
Requesting critique on something I've made

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.

Lua Code:
  1. -- Configuration
  2. local PvPSet    = "enha_pvp"
  3. local PvESet    = "enha"
  4. local msg       = "I think you might have the wrong gear equipped... :("
  5. local point     = "CENTER"
  6. local ofsx      = 0
  7. local ofsy      = 100
  8. local msgfont   = "GameFontHighlight"
  9. -- End of configuration
  11. local frame = CreateFrame("Frame")
  13. -- Register events
  14. frame:RegisterEvent("PLAYER_ENTERING_WORLD")
  15. frame:RegisterEvent("ZONE_CHANGED_NEW_AREA")
  16. frame:RegisterEvent("PLAYER_EQUIPMENT_CHANGED")
  18. -- Create the frame
  19. MyAddon_eqframe = CreateFrame("Frame",nil,UIParent)
  20. MyAddon_eqframe:SetFrameStrata("BACKGROUND")
  21. MyAddon_eqframe:SetWidth(1)
  22. MyAddon_eqframe:SetHeight(1)
  23. MyAddon_eqframe:SetPoint(point,ofsx,ofsy)
  25. -- Create the fontstring
  26. local fs = MyAddon_eqframe:CreateFontString(nil, "OVERLAY", msgfont)
  27. fs:SetPoint("CENTER")
  28. fs:SetText(msg)
  30. -- Check if the EquipSet and current equipment is the same
  31. function MyAddon_isGearEqSet(equipset)
  32.     -- Put the items from an EquipSet in a table
  33.     if equipset then
  34.         local itemArray = GetEquipmentSetItemIDs(equipset) 
  35.         local itemArrayEqSet = {}
  36.         for i=1,19 do
  37.             if itemArray[i] then
  38.                 if i ~= 4 and i ~= 18 and i ~= 19 then -- Ignore shirt, tabard and ranged slot (4 = Shirt, 18 = Ranged and 19 = Tabard)
  39.                     table.insert(itemArrayEqSet,i,itemArray[i])    
  40.                 end
  41.             end
  42.         end
  44.         -- Put your character's equipment in a table
  45.         local itemArrayPlayerEq = {}
  46.         for i=1,19 do
  47.             if i ~= 4 and i ~= 18 and i ~= 19 then -- Ignore shirt, tabard and ranged slot (4 = Shirt, 18 = Ranged and 19 = Tabard)
  48.                 table.insert(itemArrayPlayerEq,i,GetInventoryItemID("player", i))
  49.             end
  50.         end
  52.         -- Compare the
  53.         if #itemArrayEqSet ~= #itemArrayPlayerEq then
  54.             return false
  55.         end
  56.         for i=1,#itemArrayEqSet do
  57.             if itemArrayEqSet[i] ~= itemArrayPlayerEq[i] then
  58.                 return false
  59.             end
  60.         end
  61.         return true    
  62.     end
  63. end
  65. -- Look for what EquipSet you are wearing
  66. function MyAddon_getGearEqSet()
  67.     local count = GetNumEquipmentSets()
  68.     for i = 1,count do
  69.         local eqSetName = GetEquipmentSetInfo(i)
  70.         if MyAddon_isGearEqSet(eqSetName) then
  71.             return eqSetName
  72.         end
  73.     end
  74. end
  76. frame:SetScript("OnEvent", function(self, event, ...)
  78.     fs:Hide()
  80.     -- Figure out what type of zone you're in
  81.     local _, zoneType = IsInInstance();
  82.     local pvpType = GetZonePVPInfo();
  83.     if zoneType == "arena" or zoneType == "pvp" or pvpType == "combat" then
  84.         if MyAddon_getGearEqSet() == PvPSet then
  85.             fs:Hide()
  86.         else
  87.             fs:Show()
  88.         end
  89.         -- You're in a PvP area, do things here
  90.     elseif zoneType == "party" or zoneType == "raid" then
  91.         if MyAddon_getGearEqSet() == PvESet then
  92.             fs:Hide()
  93.         else
  94.             fs:Show()
  95.         end
  96.         -- You're in a party or raid, do things here
  97.     elseif zoneType == "none" then
  98.         -- You're not in an instance, do things here
  99.     else
  100.         -- You're in some other zone, do things here
  101.     end
  103. end)

  Reply With Quote
01-27-14, 02:39 PM   #2
A Chromatic Dragonspawn
Choonstertwo's Avatar
AddOn Author - Click to view addons
Join Date: Jan 2011
Posts: 194
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.

Last edited by Choonstertwo : 01-27-14 at 02:48 PM.
  Reply With Quote
01-28-14, 11:15 AM   #3
A Deviate Faerie Dragon
Join Date: Mar 2008
Posts: 10

You've been very informative.
  Reply With Quote

WoWInterface » Developer Discussions » General Authoring Discussion » Requesting critique on something I've made

Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

vB code is On
Smilies are On
[IMG] code is On
HTML code is Off