Any tips for Improving the code its kinda messy and confusing

Help and Feedback Code Review
1

Include a standalone, bare-bones rbxl file with only the code you want reviewed.

  • Code Review is for reviewing specific parts of your code, and not your whole game.
  • Code Review is intended for improving already-working code. If you need help debugging your code, please use Scripting Support.

Provide an overview of:

  • What does the code do and what are you not satisfied with?
    the code its mean so when a part touches anoter whit an specific name it gives you points and also activate some other effects and i want to make an easier way to detect which mineral is touching
  • What potential improvements have you considered?
    less code so i can be changed more easily and not be repetitive
  • How (specifically) do you want to improve the code?
    I want it to be less messy so i can change it for the future
local sound1 = script.Parent.BreakSound
local sound2 = script.Parent.BreakSound2
local sounds = {sound1,sound2}
local player = game.Players:GetPlayerFromCharacter(script.Parent.Parent.Parent.Parent)

function onTouch(hit)
	if hit.Parent.Name:match("Rock") then
		player.leaderstats.Coins.Value = player.leaderstats.Coins.Value+2
		print('Player mined a '.. hit.Parent.Name)
		sounds[math.random(1, #sounds)]:Play()
		script.Parent.RockBreaking:Play()
		script.Disabled = true
		hit.Parent.MovingModel.Disabled = false
		hit.Parent.Particles.Disabled = false
		hit.Parent.Life.Value = hit.Parent.Life.Value-25
		if hit.Parent.Life.Value < 1 then
			player.leaderstats.Coins.Value = player.leaderstats.Coins.Value+20
			script.Parent.RockFinalBreak:Play()
			local fog = game.ServerStorage.RockBreakedFog:Clone()
			fog.Parent = workspace
			fog.Position = hit.Parent.pos.Position
			fog.Script.Enabled = true
			hit.Parent:Destroy()
		end
	end
	if hit.Parent.Name:match("Metal") then
		player.leaderstats.Coins.Value = player.leaderstats.Coins.Value+3
		print('Player mined a '.. hit.Parent.Name)
		sounds[math.random(1, #sounds)]:Play()
		script.Parent.RockBreaking:Play()
		script.Disabled = true
		hit.Parent.MovingModel.Disabled = false
        hit.Parent.Particles.Disabled = false
		hit.Parent.Life.Value = hit.Parent.Life.Value-25
		if hit.Parent.Life.Value < 1 then
			player.leaderstats.Coins.Value = player.leaderstats.Coins.Value+35
			script.Parent.RockFinalBreak:Play()
			local fog = game.ServerStorage.RockBreakedFog:Clone()
			fog.Parent = workspace
			fog.Position = hit.Parent.pos.Position
			fog.Script.Enabled = true
			hit.Parent:Destroy()
		end
	end
	end
script.Parent.Touched:Connect(onTouch)

sorry if the code is messy im kinda new to triying to improve my code

1 Like
2

Put all the code that runs for both the metal and rock in a seperate function that will be called at the end of your if statements after you run the code that is unique to either the rock or the metal. Just call the function with the parameter of the object that hit it.

3

Thanks for the suggestion im working on it!

4

This is the final code thanks its so much easier to add more minerals now thanks.

local sound1 = script.Parent.BreakSound
local sound2 = script.Parent.BreakSound2
local sounds = {sound1,sound2}
local player = game.Players:GetPlayerFromCharacter(script.Parent.Parent.Parent.Parent)

function onTouch(hit)
	local function Rock()
	print('Player mined a '.. hit.Parent.Name)
	player.leaderstats.Coins.Value = player.leaderstats.Coins.Value+2
	sounds[math.random(1, #sounds)]:Play()
	script.Parent.RockBreaking:Play()
	script.Disabled = true
	hit.Parent.MovingModel.Disabled = false
	hit.Parent.Particles.Disabled = false
	hit.Parent.Life.Value = hit.Parent.Life.Value-25

	if hit.Parent.Life.Value < 1 then

		player.leaderstats.Coins.Value = player.leaderstats.Coins.Value+20
		script.Parent.RockFinalBreak:Play()
		local fog = game.ServerStorage.RockBreakedFog:Clone()
		fog.Parent = workspace
		fog.Position = hit.Parent.pos.Position
		fog.Script.Enabled = true
		hit.Parent:Destroy()
	end
end
	
	local function Metal()
		
		print('Player mined a '.. hit.Parent.Name)
		player.leaderstats.Coins.Value = player.leaderstats.Coins.Value+3
		sounds[math.random(1, #sounds)]:Play()
		script.Parent.RockBreaking:Play()
		script.Disabled = true
		hit.Parent.MovingModel.Disabled = false
		hit.Parent.Particles.Disabled = false
		hit.Parent.Life.Value = hit.Parent.Life.Value-25
		
		if hit.Parent.Life.Value < 1 then
		
			player.leaderstats.Coins.Value = player.leaderstats.Coins.Value+35
			script.Parent.RockFinalBreak:Play()
			local fog = game.ServerStorage.RockBreakedFog:Clone()
			fog.Parent = workspace
			fog.Position = hit.Parent.pos.Position
			fog.Script.Enabled = true
			hit.Parent:Destroy()
		end
	end
	
	if hit.Parent.Name:match("Rock") then
		Rock()
	end
	
if hit.Parent.Name:match("Metal") then
	Metal()
	end
end

script.Parent.Touched:Connect(onTouch)
5

Glad I could help, maybe mark mine as a solution i’m tryna build some credibility :heart:

1 Like
6 
local sound1 = script.Parent.BreakSound
local sound2 = script.Parent.BreakSound2
local sounds = {sound1,sound2}
local player = game.Players:GetPlayerFromCharacter(script.Parent.Parent.Parent.Parent)

function onTouch(hit)
	local mineralInformations = {
		Rock = {
			NormalReward = 2;
			SpecialReward = 20;
			LifeCost = 25;
		};
		
		Metal = {
			NormalReward = 3;
			SpecialReward = 35;
			LifeCost = 25;
		};
	}
	
	for mineralName, mineralInformation in mineralInformations do
		if not hit.Parent.Name:match(mineralName) then
			continue
		end
		
		player.leaderstats.Coins.Value += mineralInformation.NormalReward
		
		print("Player mined a", hit.Parent)
		
		sounds[math.random(#sounds)]:Play()
		script.Parent.RockBreaking:Play()
		
		script.Disabled = true
		hit.Parent.MovingModel.Disabled = false
		hit.Parent.Particles.Disabled = false
		hit.Parent.Life.Value -= mineralInformation.LifeCost
		
		if hit.Parent.Life.Value < 1 then
			player.leaderstats.Coins.Value += mineralInformation.SpecialReward
			script.Parent.RockFinalBreak:Play()
			
			local fog = game.ServerStorage.RockBreakedFog:Clone()
			fog.Parent = workspace
			fog.Position = hit.Parent.pos.Position
			fog.Script.Enabled = true
			hit.Parent:Destroy()
		end
		
		break
	end
end

script.Parent.Touched:Connect(onTouch)

You can use -=, +=, etc. instead of doing:

variable += --This does the same thing as below but shorter and more readable.
variable = variable + 1

You can also add a dictionary mineralInformations to easily add more minerals and its data.

1 Like
7

Tip: you can ask ChatGPT and it provides a general direction on how to improve the code. The code will probably not work as intended but you can see a clearer script which you can modify since the AI is in the right track. I wouldn’t really recommend using this AI for difficult code.

To get answers you can write:

“How can I improve this code?”

and then put the rest of the code after it.

If it doesn’t provide an example ask it

“Can you show me an example?”

If the code does not work as imagined you can ask follow up questions or modify it yourself which I imagine to be easier.

For example, here is the answer it provided: (Didn’t work as intended but it was on the right track so I modified the code which can be seen below the following script)

-- Extract the logic for updating the player's coin count into a function
local function updateCoinCount(player, rockType, value)
    player.leaderstats.Coins.Value = player.leaderstats.Coins.Value + value
end

-- Extract the logic for playing the sound into a function
local function playSound(sounds)
    sounds[math.random(1, #sounds)]:Play()
end

local sound1 = script.Parent.BreakSound
local sound2 = script.Parent.BreakSound2
local sounds = {sound1, sound2}
local player = game.Players:GetPlayerFromCharacter(script.Parent.Parent.Parent.Parent)

function onTouch(hit)
    -- Use a dictionary to map rock names to their corresponding values
    local rockValues = {
        ["Rock"] = 2,
        ["Metal"] = 3
    }

    local rockType = hit.Parent.Name
    local value = rockValues[rockType]

    -- Update the player's coin count
    updateCoinCount(player, rockType, value)

    print(string.format("Player mined a %s", rockType))

    -- Play the sound
    playSound(sounds)

    script.Parent.RockBreaking:Play()
    script.Disabled = true
    hit.Parent.MovingModel.Disabled = false
    hit.Parent.Particles.Disabled = false
    hit.Parent.Life.Value = hit.Parent.Life.Value - 25

    if hit.Parent.Life.Value < 1 then
        -- Update the player's coin count
        updateCoinCount(player, rockType, value)

        script.Parent.RockFinalBreak:Play()

        -- Create and configure the fog object
        local fog = game.ServerStorage.RockBreakedFog:Clone()
        fog.Parent = workspace
        fog.Position = hit.Parent.pos.Position
        fog.Script.Enabled = true

        hit.Parent:Destroy()
    end
end

script.Parent.Touched:Connect(onTouch)

I modified the code to work as intended:

local sound1 = script.Parent.BreakSound
local sound2 = script.Parent.BreakSound2
local sounds = {sound1, sound2}
local player = game.Players:GetPlayerFromCharacter(script.Parent.Parent.Parent.Parent)

-- Use a dictionary to map rock names to their corresponding values
local rockValues = {
	["Rock"] = {2, 20}, -- First value is regular increase, second value is after rock is broken.
	["Metal"] = {3, 35},
	["ExampleValue"] = {4, 50} -- You can easily add more values to the dictionary
}

-- Extract the logic for updating the player's coin count into a function
local function updateCoinCount(player, rockType, value)
	player.leaderstats.Coins.Value = player.leaderstats.Coins.Value + value
end

-- Extract the logic for playing the sound into a function
local function playSound(sounds)
	sounds[math.random(1, #sounds)]:Play()
end

function onTouch(hit)
	local rockType = hit.Parent.Name
	local value = rockValues[rockType]
	-- If hit.Parent.Name doesn't exist in the dictionary stop the code from executing
	if not value then return end

	-- Update the player's coin count
	updateCoinCount(player, rockType, value[1])

	print(string.format("Player mined a %s", rockType))

	-- Play the sound
	playSound(sounds)

	script.Parent.RockBreaking:Play()
	script.Disabled = true
	hit.Parent.MovingModel.Disabled = false
	hit.Parent.Particles.Disabled = false
	hit.Parent.Life.Value = hit.Parent.Life.Value - 25

	if hit.Parent.Life.Value < 1 then
		-- Update the player's coin count
		updateCoinCount(player, rockType, value[2])

		script.Parent.RockFinalBreak:Play()

		-- Create and configure the fog object
		local fog = game.ServerStorage.RockBreakedFog:Clone()
		fog.Parent = workspace
		fog.Position = hit.Parent.pos.Position
		fog.Script.Enabled = true

		hit.Parent:Destroy()
	end
end

script.Parent.Touched:Connect(onTouch)
1 Like
8

Having multiple solutions doesn’t mean you gain credibility.

1 Like
9

Who would you trust more, someone with 100 solutions or 3. simple as that

10

Past contributions actually mean something though. A number doesn’t determine if you are good enough for something.

11

This topic was automatically closed 14 days after the last reply. New replies are no longer allowed.