Quantcast Critique my code! - WoWInterface
Thread Tools Display Modes
08-20-19, 06:25 PM   #1
ChandlerJoseph
A Deviate Faerie Dragon
Join Date: Aug 2019
Posts: 14
Critique my code!

I am new to lua and coding in general so any feedback on these 2 small addons is appreciated.

Context: Hides minimap buttons until mouseover.
https://pastebin.com/gdcXHF5k

Context: Hides the enitre micro menu until mouseover.
https://pastebin.com/aUh1b9Zp
  Reply With Quote
08-21-19, 07:41 AM   #2
fusionpit
A Murloc Raider
AddOn Author - Click to view addons
Join Date: Apr 2009
Posts: 6
Originally Posted by ChandlerJoseph View Post
I am new to lua and coding in general so any feedback on these 2 small addons is appreciated.

Context: Hides minimap buttons until mouseover.
https://pastebin.com/gdcXHF5k

Context: Hides the enitre micro menu until mouseover.
https://pastebin.com/aUh1b9Zp
You have variables named things like "DHMB" and "DHMM" which aren't clear until you read the code and see they're a timer table way down near the bottom. Same with "Ignore" in the Micro Button one. Ignore WHAT? I shouldn't have to find where the variable is used in order to know what its purpose is.

So, come up with "better", clearer names for them. It'll probably be hard, because the two hardest things in software development are regular expressions, naming things, and off-by-one errors.

Also, I believe convention for local variables/functions and function params is camelCase. Not that that point matters much, as long as you're consistent in which casing you use.
  Reply With Quote
08-21-19, 08:42 AM   #3
Terenna
A Warpwood Thunder Caller
AddOn Author - Click to view addons
Join Date: Jun 2016
Posts: 93
Any instance of "in pairs" loops:
Lua Code:
  1. for _, Frame in pairs(MinimapButtons) do
  2.     Frame:SetAlpha(0)
  3. end

should be written as:
Lua Code:
  1. for i = 1, #MinimapButtons do
  2.     MinimapButtons[i]:SetAlpha(0)
  3. end

or something similar. "in pairs" uses more CPU than just running through the table's values with an integer for loop.
  Reply With Quote
08-21-19, 10:52 AM   #4
ChandlerJoseph
A Deviate Faerie Dragon
Join Date: Aug 2019
Posts: 14
Would you mind explaining a little further the differences between what I did and what you suggested?
  Reply With Quote
08-21-19, 12:57 PM   #5
Lybrial
A Theradrim Guardian
AddOn Compiler - Click to view compilations
Join Date: Jan 2010
Posts: 66
He is basically saying that an integer loop is faster than ipairs. But... to be honest...
that really does not matter. You are not doing any stuff which needs a optimized
performance. Your code is fine. The only thing I would criticize is that you are not
covering every minimap button, only those in your table. You could do something like this
to get every button:

Lua Code:
  1. local excluded = {};
  2. local buttons = {};
  3.  
  4. local function initButtons()
  5.     for _, frame in pairs({ Minimap, MinimapBackdrop }) do
  6.         local children = frame:GetNumChildren();
  7.  
  8.         for i = 1, children do
  9.             local button = select(i, frame:GetChildren());
  10.  
  11.             if (isValidButton(button) then
  12.                 tinsert(self.buttons, button);
  13.             end
  14.         end
  15.     end
  16. end
  17.  
  18. -- validation could be something like:
  19. local function isValidButton(button)
  20.     if (not button) then
  21.         return false;
  22.     end
  23.  
  24.     if (not button:IsVisible()) then
  25.         return false;
  26.     end
  27.  
  28.     if (not button:GetName()) then
  29.         return false;
  30.     end
  31.  
  32.     if (not (button:GetWidth() > 14 and button:GetWidth() < 55)) then
  33.         return false;
  34.     end -- questionable restriction
  35.  
  36.     if (not (button:IsObjectType("Button") or button:IsObjectType("Frame"))) then
  37.         return false;
  38.     end
  39.  
  40.     -- exclude those buttons, which are in the excluded list
  41.     for _, value in pairs(excluded) do
  42.         if (string.find(button:GetName(), value)) then
  43.             return false;
  44.         end
  45.     end
  46.  
  47.     return true;
  48. end

In general I would always recommend to choose speaking variables and function names.
Also I would recommend anyone who starts to code in any programming language to
get to know the basic programming and engineering skills.

Originally Posted by fusionpit View Post
It'll probably be hard, because the two hardest things in software development are regular expressions, naming things, and off-by-one errors.
THIS!

Last edited by Lybrial : 08-21-19 at 01:01 PM.
  Reply With Quote
08-21-19, 08:10 PM   #6
ChandlerJoseph
A Deviate Faerie Dragon
Join Date: Aug 2019
Posts: 14
But it seems more complicated to do what youre doing vs just grabbing the frame and executing the functions
  Reply With Quote
08-21-19, 09:24 PM   #7
ChandlerJoseph
A Deviate Faerie Dragon
Join Date: Aug 2019
Posts: 14
whats more efficient and why?

for i = 1, #MinimapButtons do
MinimapButtons[i]:SetAlpha(0)
end

for _, Frame in pairs(MinimapButtons) do
Frame:SetAlpha(0)
end
  Reply With Quote
08-21-19, 09:35 PM   #8
Terenna
A Warpwood Thunder Caller
AddOn Author - Click to view addons
Join Date: Jun 2016
Posts: 93
The integer loop, it doesn't perform a function call, pairs, which actually calls another function.
  Reply With Quote
08-21-19, 09:39 PM   #9
ChandlerJoseph
A Deviate Faerie Dragon
Join Date: Aug 2019
Posts: 14
okay, thanks for the reply, for the integer loop, is it infinite or does it stop at the end of the table?
  Reply With Quote
08-22-19, 12:27 AM   #10
Lybrial
A Theradrim Guardian
AddOn Compiler - Click to view compilations
Join Date: Jan 2010
Posts: 66
Originally Posted by ChandlerJoseph View Post
okay, thanks for the reply, for the integer loop, is it infinite or does it stop at the end of the table?
It stops at the end of the table:

Lua Code:
  1. for i = 1, #MinimapButtons do

"#MinimapButtons" means the amount of values in MinimapButtons. So if you have 10 minimap buttons it would be:

Lua Code:
  1. for i = 1, 10 do


But it seems more complicated to do what youre doing vs just grabbing the frame and executing the functions
Ofcourse it is more complexe but it also provides much more functionality. Thats your decision.
If you use that code only for yourself you dont need that. But if you are going to public an addon
for anyone your code would be quite useless if other players have minimap buttons that you dont have.
  Reply With Quote
08-23-19, 01:10 PM   #11
Seerah
Fishing Trainer
 
Seerah's Avatar
WoWInterface Super Mod
Featured
Join Date: Oct 2006
Posts: 10,637
Note that an integer loop is the same as ipairs(), but not the same as pairs(). The latter can be used when there are holes in your table or when your table does not have integer-based keys.
__________________
"You'd be surprised how many people violate this simple principle every day of their lives and try to fit square pegs into round holes, ignoring the clear reality that Things Are As They Are." -Benjamin Hoff, The Tao of Pooh

  Reply With Quote
08-23-19, 04:16 PM   #12
ChandlerJoseph
A Deviate Faerie Dragon
Join Date: Aug 2019
Posts: 14
so youre saying if my table is for instance containing {PlayerFrame, FocusFrame, TargetFrame}. I should use pairs() or the numeric loop?
  Reply With Quote
08-23-19, 06:14 PM   #13
fusionpit
A Murloc Raider
AddOn Author - Click to view addons
Join Date: Apr 2009
Posts: 6
That person is saying "for i = 1, #table do" and "for k, v in ipairs(table) do" are basically the same, with the first being slightly more performant. If you had a table that looked like this:
Lua Code:
  1. {
  2.     firstFrame = PlayerFrame,
  3.     secondFrame = FocusFrame,
  4.     thirdFrame = TargetFrame
  5. }
or
Lua Code:
  1. {PlayerFrame, nil, FocusFrame, TargetFrame}
then you would need to use pairs, since ipairs will only iterate over numerical indexes (starting at 1) up to a nil value.

In the case of the second example,
Lua Code:
  1. for k,v in ipairs({PlayerFrame, nil, FocusFrame, TargetFrame}) do
  2.    print(k, v)
  3. end
will output
Code:
1, table:...
while
Lua Code:
  1. for k,v in pairs({PlayerFrame, nil, FocusFrame, TargetFrame}) do
  2.    print(k, v)
  3. end
will output
Code:
1, table:...
3, table:...
4, table:...

Last edited by fusionpit : 08-23-19 at 08:21 PM.
  Reply With Quote

WoWInterface » Developer Discussions » Lua/XML Help » Critique my code!

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