Good scripting or not?

Help and Feedback Code Review
#1

Hello,

Sorry for my bad english.

I wan’t to ask if its good if i script like that:

--// VARIABLES //--
local Detection_Part = script.Parent;
local Click_Detector = Detection_Part:FindFirstChild('Click_Detector');
local Settings_Folder = Detection_Part:FindFirstChild('Settings');
local Delay = Settings_Folder:FindFirstChild('Delay');
local Can_Click = true;

--// FUNCTION //--
Click_Detector.MouseClick:Connect(function(Who_Clicked)
      if (Can_Click == true) then
          Can_Click = false;
          print('Who_Clicked.Name');
          wait(Delay.Value);
          Can_Click = true;
      end
end)

I mean if is good to use “;” and “()” in “if” functions in LUA?

2 Likes
#2

I don’t think it makes much difference, it’s all down to your personal habits/preference.

2 Likes
#3

This is what #development-support:code-review is for. As for what you asked, parenthesis matter contextually but not semicolons. The latter is preference, the former has a few constraints on it.

Aside from that, I’d probably use WaitForChild over FindFirstChild. WaitForChild has FindFirstChild built-in as the first call. If it’s not found, it pauses the script until the object is found.

8 Likes
#4

Okey thank you for your time :slight_smile:

#5

Your code looks fine. If you plan on working for other developers as a coder in the future, you may want to consider conforming to a more universal style.

Also I noticed you have print('Who_Clicked.Name'). On that line, it looks like you’re trying to print out the player’s name. But it is wrapped in quotes, so all you are printing is literally Who_Clicked.Name.

Here’s how I would write that code, just for an example:

local detectionPart = script.Parent
local clickDetector = detectionPart:FindFirstChild("Click_Detector")
local settingsFolder = detectionPart:FindFirstChild("Settings")
local delay = settingsFolder:FindFirstChild("Delay")

local canClick = true

clickDetector.MouseClick:connect(function(whoClicked)
    if canClick == true then
        canClick = false
        print(whoClicked.Name)
        wait(delay.Value)
        canClick = true
    end
end)

Semicolons are totally optional as well, just comes down to your preferred style. I usually hold off on comments for things that are obvious, but they are very nice when doing something intricate and somewhat confusing.

5 Likes
#6

On the wiki it specifically states that WaitForChild is less efficient that the dot operator or FindFirstChild. Furthermore, the article for FindFirstChild also states that it is less efficient that the dot operator. Using FindFirstChild over the dot operator only makes sense if you don’t want it to wait for the child and you have to make sure the child exists. This makes simply indexing the instances for the variables the best method, as there is no checks for if the instances exist.

This could be changed to

local clickDetector = detectionPart.Click_Detector

Edit: WaitForChild is really only necessary if you need the instance and you can’t guarantee it exists, i.e getting an object from workspace with a script in ReplicatedFirst

1 Like
#7

It doesn’t matter which one is more efficient, it’s use is case dependent. If this is being run on the client, you are almost definitely going to want to include WaitForChild on at least one of the assembly’s ancestors to ensure that the descendant instances that are implicitly copied along with the ancestor exist.

If this is being run on the server and it’s an object which doesn’t have it’s ancestry modified at any point, neither WaitForChild or FindFirstChild really need to be used. FindFirstChild is useful for if you need to check existence only once, WaitForChild when you need to yield until an object is found (or cancel that out using the timeout parameter and move on).

The difference in performance is marginal in the first place and you won’t experience any visible performance differences. Sacrificing the use of a method in a case where it belongs over a (near-)microoptimisation is pointless.

3 Likes