WoWInterface

WoWInterface (https://www.wowinterface.com/forums/index.php)
-   General Authoring Discussion (https://www.wowinterface.com/forums/forumdisplay.php?f=20)
-   -   Requesting critique on something I've made (https://www.wowinterface.com/forums/showthread.php?t=48878)

wiMp 01-27-14 01:13 PM
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
  10.  
  11. local frame = CreateFrame("Frame")
  12.  
  13. -- Register events
  14. frame:RegisterEvent("PLAYER_ENTERING_WORLD")
  15. frame:RegisterEvent("ZONE_CHANGED_NEW_AREA")
  16. frame:RegisterEvent("PLAYER_EQUIPMENT_CHANGED")
  17.  
  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)
  24.  
  25. -- Create the fontstring
  26. local fs = MyAddon_eqframe:CreateFontString(nil, "OVERLAY", msgfont)
  27. fs:SetPoint("CENTER")
  28. fs:SetText(msg)
  29.  
  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
  43.         
  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
  51.         
  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
  64.  
  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
  75.  
  76. frame:SetScript("OnEvent", function(self, event, ...)
  77.  
  78.     fs:Hide()
  79.     
  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
  102.     
  103. end)

Thanks!

Choonstertwo 01-27-14 02:39 PM
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 01-28-14 11:15 AM
Thanks!

You've been very informative.


All times are GMT -6. The time now is 12:58 PM.

vBulletin © 2024, Jelsoft Enterprises Ltd
© 2004 - 2022 MMOUI