I'm a beginner, I'm interested if I could make my code look nicer in any way (Mining script)

Help and Feedback Code Review
1

I made a tool (a pickaxe) with which you click on specific parts, and after 10 seconds, they get moved to ReplicatedStorage (mined) and regenerate after 15 seconds. I’m just interested to know if there’s some cool tips or tricks I could do to make my code neater. As I said, I’m fairly new (been learning for around a week now), I did look at tutorials and ask for help and stuff, but I didn’t just copy-paste, I understand how and why this works.

local tool = script.Parent
local handle = tool:WaitForChild("Handle")
local plr = game.Players.LocalPlayer
local mouse = plr:GetMouse()
local debounce = false
local nodes = {"StoneNode","CoalNode"}


function isEquipped()
	if tool.Parent.Name ~= "Backpack" then
		return true
	else
		return false
	end
end

mouse.Button1Down:Connect(function()
	local inventoryGui = plr.PlayerGui.Main.Inventory
	if isEquipped() and not debounce then
		local target = mouse.Target
		for i, v in pairs(nodes) do
		if target.Name == v then
			debounce = true
			
			--Stone Node Mined
			if v == "StoneNode" then
					for i = 10, 1, -1 do
					print(i)
					wait(1)
					if i == 1 then
						target.Parent = game.ReplicatedStorage
						--WIP
					end
				end
				--Regenerating
				delay(15, function()
					target.Parent = game.Workspace
				end)
			end
			
			--Coal Node Mined
			if v == "CoalNode" then
					for i = 10, 1, -1 do
					print(i)
					wait(1)
					if i == 1 then
						target.Parent = game.ReplicatedStorage
						--WIP
					end
				end
				--Regenerating
				delay(15, function()
					target.Parent = game.Workspace
				end)
			end
			
			debounce = false
			end
		end
	end
end)
5 Likes
3

Can you edit the original post to have three backticks (```--your code here```) around your code? It’ll be much easier to critique if it’s easily readable on the forum.

4 Likes
4

You may of seen this before but comments to segment the entire script into dependencies, variables, functions, connections.
Example:

--// Dependencies
local players = game:GetService("Players")

--// Variables
local plr = players.LocalPlayer

--// Functions
function hello(character)
    print("Hello", character.Name, "!")
end

--// Connections
plr.CharacterAdded:Connect(hello)
3 Likes
5

Most of the code actually looks great! My only critique is that the contents of the for loop aren’t indented in. My rule of thumb is that any code inside a block is indented one tab (or however many spaces).


mouse.Button1Down:Connect(function()
local inventoryGui = plr.PlayerGui.Main.Inventory
if isEquipped() and not debounce then
  local target = mouse.Target
  for i, v in pairs(nodes) do
    if target.Name == v then
      debounce = true

      --Stone Node Mined
      if v == "StoneNode" then
        for i = 10, 1, -1 do
          print(i)
          wait(1)
          if i == 1 then
            target.Parent = game.ReplicatedStorage
            --WIP
          end
        end
        --Regenerating
        delay(15, function()
        target.Parent = game.Workspace
        end)
      end

      --Coal Node Mined
      if v == "CoalNode" then
        for i = 10, 1, -1 do
          print(i)
          wait(1)
          if i == 1 then
            target.Parent = game.ReplicatedStorage
            --WIP
          end
        end
        --Regenerating
        delay(15, function()
        target.Parent = game.Workspace
        end)
      end

      debounce = false
    end
  end
end
end)

If you’d like you can find many code beautifiers online. Just paste your code it and run the program. Not sure about posting outside links but if you google “Lua code beautifier” you should find plenty of results.

5 Likes
6

The style is good (aside from a couple indentation errors), though the program structure itself is questionable in a few places.

You have repeated code for handling StoneNode and CoalNode - at the moment the code is completely identical, so the if statement is unnecessary. This will probably change in future (perhaps mining a node adds an item to your inventory, depending on the type), but it’s still worth handling the common functionality in one place, so if you want to change something like the respawn time, you only need to change one number.

for i = 10, 1, -1 do
    print(i)
    wait(1)
    if i == 1 then
        target.Parent = game.ReplicatedStorage
        --WIP
end

This is an extraordinarily roundabout way of simply doing this:

wait(10)
target.Parent = game.ReplicatedStorage

I’m not sure what the for loop is for (looks to be the remains of some old functionality that you removed), though since it’s redundant it should be removed to make it easier to follow the program’s flow.

Secondly, while this is more subjective, I’d avoid using delay here:

--move part after 10 seconds
delay(15, function()
    target.Parent = game.Workspace
end)
debounce = false

Starting a new thread does work, though it would be simpler to just clear the debounce after the part has been reparented, like this:

--move part after 10 seconds
debounce = false
wait(15)
target.Parent = game.Workspace

That being said, you could argue that using delay makes the asynchronous behaviour more explicit, so you could go either way here.

Whilst not directly relevant to the question, I’d also like to point out that since the entire system is in a localscript, the parts will only be removed on the client side - the parts won’t be removed for other players.
This might be intentional, though I figured I’d point it out.

1 Like
7

“This will probably change in future (perhaps mining a node adds an item to your inventory, depending on the type)” You’re absolutely right, it changes an intvalue in the player, so the if statement is necessary.

“I’m not sure what the for loop is for (looks to be the remains of some old functionality that you removed), though since it’s redundant it should be removed to make it easier to follow the program’s flow.” I made the for loop for the future, I’m going to be making a timer gui, where it counts down from 10 seconds.

About the delay, I did it because if I do a wait, then it doesn’t continue on with the script until that’s over, making it so I can’t mine another node until the wait is over, I don’t want that to happen.

“Whilst not directly relevant to the question, I’d also like to point out that since the entire system is in a localscript, the parts will only be removed on the client side - the parts won’t be removed for other players.” That’s not something I thought about, and it isn’t intentional, I’ll have to fix it, thanks for pointing it out!

Big thanks for your reply :slight_smile:

8 
function isEquipped()
	if tool.Parent.Name ~= "Backpack" then
		return true
	else
		return false
	end
end

Instead of checking where the tool is and using Mouse.Button1Down, You can use the Tool.Activated event.
This event fires when you click while holding a tool so, You will get the same result but shorter.

1 Like