WoWInterface

WoWInterface (https://www.wowinterface.com/forums/index.php)
-   Lua/XML Help (https://www.wowinterface.com/forums/forumdisplay.php?f=16)
-   -   Critique my code! (https://www.wowinterface.com/forums/showthread.php?t=57376)

ChandlerJoseph 08-20-19 06:25 PM

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

fusionpit 08-21-19 07:41 AM

Quote:

Originally Posted by ChandlerJoseph (Post 333293)
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.

Terenna 08-21-19 08:42 AM

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.

ChandlerJoseph 08-21-19 10:52 AM

Would you mind explaining a little further the differences between what I did and what you suggested?

Lybrial 08-21-19 12:57 PM

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.

Quote:

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

THIS!

ChandlerJoseph 08-21-19 08:10 PM

But it seems more complicated to do what youre doing vs just grabbing the frame and executing the functions

ChandlerJoseph 08-21-19 09:24 PM

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

Terenna 08-21-19 09:35 PM

The integer loop, it doesn't perform a function call, pairs, which actually calls another function.

ChandlerJoseph 08-21-19 09:39 PM

okay, thanks for the reply, for the integer loop, is it infinite or does it stop at the end of the table?

Lybrial 08-22-19 12:27 AM

Quote:

Originally Posted by ChandlerJoseph (Post 333303)
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


Quote:

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.

Seerah 08-23-19 01:10 PM

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.

ChandlerJoseph 08-23-19 04:16 PM

so youre saying if my table is for instance containing {PlayerFrame, FocusFrame, TargetFrame}. I should use pairs() or the numeric loop?

fusionpit 08-23-19 06:14 PM

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:...



All times are GMT -6. The time now is 03:34 PM.

vBulletin © 2019, Jelsoft Enterprises Ltd
© 2004 - 2019 MMOUI