How good is this cocoa code?

Help and Feedback Code Review
#1

So, here I am in the #help-and-feedback:code-review category. Is this a good use of tweening parts?

local cocao = script.Parent
local startPos = script.Parent.StartingPosition

local storage = game.ServerStorage
local tweeningService = game:GetService("TweenService")

local tweenInfo = TweenInfo.new(

15,
Enum.EasingStyle.Quint,
Enum.EasingDirection.Out,
0,
false,
0

)

local startProperties = {

Size = script.Parent.StartingSize.Value,
Position = script.Parent.StartingPosition.Value

}

local tween = tweeningService:Create(cocao,tweenInfo,startProperties)

-- bools
local cocoaCollected = false
local cocoaAbleToCollect = true

local deb = false

local function collectCocoa(hit)
	if not deb then
		deb = true
		local humanoid = hit.Parent:FindFirstChild("Humanoid")
		if humanoid ~= nil then
			local player = game.Players:GetPlayerFromCharacter(hit.Parent)
			local cocoaStat = player.leaderstats:WaitForChild("Cocoa")
			if cocoaStat ~= nil then
				if cocoaAbleToCollect then
					print("Made it to the collecting phase")
					cocoaStat.Value = cocoaStat.Value + (1 * player.Gamepasses.CocoaMultiplier.Value)
					
					cocoaCollected = true
					cocoaAbleToCollect = false
					
					script.Parent.Position = script.Parent.GrowingPosition.Value
					script.Parent.Size = script.Parent.GrowingSize.Value
					
					tween:Play()
					wait(15)
					
					cocoaCollected = false
					cocoaAbleToCollect = true
					
					deb = false
				end
			end
		else
			deb = false
		end
	end
end

cocao.Touched:Connect(collectCocoa)

When I first created the cocoa system (not this one) I just made it so it would fall and then the player would collect it.
Instead with this code the player will collect it from the tree and earn cocoa, and then the cocoa would start growing again, and that is where the tweening comes in.

I’m satisfied with everything, but I just want to know what you guys think of it.

  • 1
  • 2
  • 3
  • 4
  • 5

0 voters

Thank you!

2 Likes
#2

I did a few edits, so now the cocoa drops!

local cocao = script.Parent

local tweeningService = game:GetService("TweenService")
local tweenInfo = TweenInfo.new(20, Enum.EasingStyle.Linear, Enum.EasingDirection.Out, 0, false, 0)
local tweenProperties = {Size = cocao.StartSize.Value, Position = cocao.StartPos.Value}
local tween = tweeningService:Create(cocao,tweenInfo,tweenProperties)

local cocoaCollected = false
local cocoaAbleToCollect = true

local deb = false

wait(1)

cocao.Anchored = false

local function collectCocoa(hit)
	if not deb then
		deb = true
		local humanoid = hit.Parent:FindFirstChild("Humanoid")
		if humanoid ~= nil then
			local player = game.Players:GetPlayerFromCharacter(hit.Parent)
			local cocoaStat = player.leaderstats:WaitForChild("Cocoa")
			if cocoaStat ~= nil then
				if cocoaAbleToCollect then
					print("Made it to the collecting phase")
					cocoaStat.Value = cocoaStat.Value + (1 * player.Gamepasses.CocoaMultiplier.Value)
					
					cocoaCollected = true
					cocoaAbleToCollect = false
					cocao.Anchored = true
					
					cocao.Position = cocao.GrowPos.Value
					cocao.Size = cocao.GrowSize.Value
					cocao.Orientation = cocao.StartRotation.Value
					
					tween:Play()
					wait(20.5)
					
					cocoaCollected = false
					cocoaAbleToCollect = true
					cocao.Anchored = false
					deb = false
				end
			else
				warn("No cocoa stat found")
				deb = false
			end
		else
			deb = false
		end
	end
end

cocao.Touched:Connect(collectCocoa)
  • 1
  • 2
  • 3
  • 4
  • 5

0 voters

2 Likes
#3

You just have a single block of code that does all of the logic, with 5 levels of indentation. This makes the code hard to reason about, and hard to read because it might be wide that the reader’s screen.

The cocoaCollected and cocoaAbleToCollect variables do the same thing, get rid of one of them. Same with the deb “debounce” variable, it’s made completely redundant by the cocoaAbleToCollect variable. It’s also a really weird Roblox- specific (AFAIK) pattern that I’ve never seen the point of, to me it’s a code smell and I never use it because a situation where it might be useful just never comes up.

Also, the name of the collectCocoa function is misleading because that’s not actually what it does. Well, sometimes it does, but only under very specific circumstances defined by the complicated tree of if-else statements.

The magic numbers 20 and 20.5 make it hard to realize that it’s supposed to play the regrowth animation and wait for it to complete. Use the Tween.Completed event instead to make it clearer what is going on.

Here's my version of your script: 
local cocoa = script.Parent

local isCollected = false
local cooldownAnimLength = 20
local cooldownCollectionDelay = 0.5

local tweenInfo = TweenInfo.new(cooldownAnimLength, Enum.EasingStyle.Linear, Enum.EasingDirection.Out, 0, false, 0)
local tweenProperties = {Size = cocoa.StartSize.Value, Position = cocoa.StartPos.Value}
local tween = game:GetService("TweenService"):Create(cocoa, tweenInfo, tweenProperties)

local function collectCocoa(collectingPlayer)
	--Verify that player's leaderstats is correctly set up
	-- although... shouldn't we throw an error instead?	
	local cocoaStat = collectingPlayer.leaderstats:WaitForChild("Cocoa")
	if not cocoaStat then
		warn("No cocoa stat found")
		return
	end

	print(string.format("Cocoa collected by %s", collectingPlayer.Name))
	
	--Increment player's cocoa stat
	cocoaStat.Value = cocoaStat.Value + collectingPlayer.Gamepasses.CocoaMultiplier.Value
	
	--Set script state to prevent collecting before cooldown is over
	cocoaCollected = true

	--Update cocoa visuals
	cocoa.Anchored = true
	cocoa.Position = cocoa.GrowPos.Value
	cocoa.Size = cocoa.GrowSize.Value
	cocoa.Orientation = cocoa.StartRotation.Value
	
	--Play cocoa growing animation
	tween:Play()
	tween.Completed:Wait() --wait for the anim to end
	wait(cooldownCollectionDelay) --short additional delay
	
	--Reset cocoa visuals
	cocoa.Anchored = false

	--Set script state to allow collecting again
	cocoaCollected = false
end

local function getCharacterFromLimb( limb )
	local character = limb.Parent
	if character:FindFirstChild("Humanoid") then
		return character
	end

	return nil
end

local function onCocoaTouched( touchingPart )
	--Try to collect the cocoa if it was touched by a valid character
	local character = getCharacterFromLimb(touchingPart)

	if character and (not isCollected) then
		collectCocoa( game.Players:GetPlayerFromCharacter(character) )
	end
end

cocoa.Touched:Connect(onCocoaTouched)

It’s got no deeply nested if statements, and I think it’s a lot easier to follow the logic. It’s clear what exactly the conditions are for the cocoa to be collected when touched, but the exact meaning of being touched by a “player” is put in a separate function so the reader only has to read it of they care.

I hope this is helpful. Of course a lot of it is opinion-based but I really think the improvements I suggested will work.