Ok, I have code, but is it good?

Help and Feedback Scripting Support
1 
local Particles = game.ReplicatedStorage.Effects.Particles
local Highlights = game.ReplicatedStorage.Effects.Highlights
local Parts = game.ReplicatedStorage.Effects.Parts

local BindableEvents = game.ReplicatedStorage.Events.BindableEvents
local RemoteEvents = game.ReplicatedStorage.Events.RemoteEvents

local TweenService = game:GetService("TweenService")
local CollectionService = game:GetService("CollectionService")

local NewRandom = Random.new()

local IcePhysicalProperties = PhysicalProperties.new(1, 0.001, 0.5, 100, 1)
local PhysicalPropertiesList = {}

function SetPartSurfaceType(Part, SurfaceType, SettingDelay)
	task.wait(SettingDelay)
	Part.FrontSurface = SurfaceType
	Part.BackSurface = SurfaceType
	Part.TopSurface = SurfaceType
	Part.BottomSurface = SurfaceType
	Part.RightSurface = SurfaceType
	Part.LeftSurface = SurfaceType
end

CollectionService:GetInstanceAddedSignal("OnFire"):Connect(function(TagInstance)
	if TagInstance:FindFirstChild("Humanoid") then
		if not TagInstance:HasTag("ImmuneToFire") and not TagInstance:HasTag("CannotCatchFire") then
			local Humanoid = TagInstance.Humanoid
			local FireHighlight = Highlights.FireHighlight:Clone()
			FireHighlight.Parent = TagInstance
			BindableEvents.DoDamage:Fire("Fire", TagInstance, 5, 0.5, 10)
			TweenService:Create(FireHighlight, TweenInfo.new(1), {FillTransparency = 0.5, OutlineTransparency = 0}):Play()

			for i = 1, 20 do
				if not TagInstance:HasTag("OnFire") then
					break
				end
				
				task.wait(0.25)
				if Humanoid.FloorMaterial ~= Enum.Material.Air then
					Humanoid.Jump = true
				end
			end
			
			TagInstance:RemoveTag("OnFire")
		end
	else
		TweenService:Create(TagInstance, TweenInfo.new(1), {Color = Color3.fromRGB(255, 125, 0)}):Play()
		SetPartSurfaceType(TagInstance, Enum.SurfaceType.Weld, 0.3)

		local Fire = Particles.Fire:Clone()
		Fire.Rate = TagInstance.Size.Magnitude * 10
		Fire.Parent = TagInstance
		
		local TouchConnection = TagInstance.Touched:Connect(function(Hit)
			if Hit.Parent:FindFirstChild("Humanoid")
				and not Hit.Parent:HasTag("ImmuneToFire")
				and not Hit.Parent:HasTag("CannotCatchFire") then
				Hit.Parent:AddTag("OnFire")
			end
		end)
		
		local TagSignal
		
		TagSignal = game.CollectionService:GetInstanceRemovedSignal("OnFire"):Connect(function(TagInstance2)
			if TagInstance2 == TagInstance then
				TouchConnection:Disconnect()
				TagSignal:Disconnect()
			end
		end)
		
		for i = 1, 5 do
			task.wait(3)
			
			if not TagInstance:HasTag("OnFire") then
				break
			end
			
			for i, v in pairs(workspace:GetPartBoundsInBox(TagInstance.CFrame, TagInstance.Size * 1.1)) do
				if v:IsA("BasePart") and
					(not v:HasTag("OnFire")
					and not v:HasTag('CannotCatchFire') and
					not v:HasTag("Unchangeable")
					and v.Size.Magnitude < TagInstance.Size.Magnitude * 5
					and not v.Parent:FindFirstChild("Humanoid")) then
					v:AddTag("OnFire")
				end
			end
		end
		
		task.wait(1.5)
		TagInstance:RemoveTag("OnFire")
	end
end)

CollectionService:GetInstanceRemovedSignal("OnFire"):Connect(function(TagInstance)
	if TagInstance:FindFirstChild("Humanoid") then
		TagInstance.Humanoid:RemoveTag("BeingDamagedBySourceFire")
	end

	if TagInstance:FindFirstChild("Fire") then
		TagInstance:FindFirstChild("Fire"):Destroy()
	end
	
	if TagInstance:FindFirstChild("FireHighlight") then
		TweenService:Create(TagInstance:FindFirstChild("FireHighlight"), TweenInfo.new(1), {FillTransparency = 1, OutlineTransparency = 1}):Play()
	end

	if TagInstance:IsA("BasePart") then
		TweenService:Create(TagInstance, TweenInfo.new(1), {Color = Color3.fromRGB(0, 0, 0)}):Play()
	end

	TagInstance:AddTag("CannotCatchFire")

	task.wait(2)
	TagInstance:RemoveTag("CannotCatchFire")

	if TagInstance:FindFirstChild("FireHighlight") then
		TagInstance:FindFirstChild("FireHighlight"):Destroy()
	end
end)

CollectionService:GetInstanceAddedSignal("Chilled"):Connect(function(TagInstance)
	TagInstance:AddTag("ImmuneToFire")
	TagInstance:RemoveTag("OnFire")
	
	if TagInstance:FindFirstChild("Humanoid") then
		local Ice = Parts.Ice:Clone()
		Ice.CFrame = TagInstance.HumanoidRootPart.CFrame
		Ice.Anchored = false
		Ice.Parent = TagInstance
		
		local IceWeld = Instance.new("WeldConstraint")
		IceWeld.Part0 = TagInstance.HumanoidRootPart
		IceWeld.Part1 = Ice
		IceWeld.Parent = Ice
		
		TagInstance.Humanoid:RemoveTag("BeingDamagedBySourceFire")
		TagInstance:AddTag("ImmuneToFire")
		TagInstance:AddTag("Ragdolled")
	else
		PhysicalPropertiesList[TagInstance] = TagInstance.CustomPhysicalProperties
		TagInstance.CustomPhysicalProperties = IcePhysicalProperties
		TweenService:Create(TagInstance, TweenInfo.new(1), {Reflectance = 0.5}):Play()
	end
	
	task.wait(10)
	TagInstance:RemoveTag("Chilled")
end)

CollectionService:GetInstanceRemovedSignal("Chilled"):Connect(function(TagInstance)
	TagInstance:RemoveTag("ImmuneToFire")

	if TagInstance:FindFirstChild("Humanoid") then
		if TagInstance:FindFirstChild("Ice") then
			TagInstance:FindFirstChild("Ice"):Destroy()
		end
		TagInstance:RemoveTag("Ragdolled")
	else
		TagInstance.CustomPhysicalProperties = PhysicalPropertiesList[TagInstance]
		TweenService:Create(TagInstance, TweenInfo.new(1), {Reflectance = 0}):Play()
		PhysicalPropertiesList[TagInstance] = nil
	end
end)

I have code (the thing just above this text) but I want to know if it’s actually good. I’ve heard a lot about the numbers thing “ohh variablize your numbers to make code more clear” and yes, I’ll do that at some point, but other then that, is this code presentable at all? could I use this in a game without looking like I have no clue what I am doing? Is this code slow? What should I improve?

I’ve been weary of my own code and how it may look when someone else reads it back for a while, just want to know if my fears are founded.

7 Likes
2

Type annotation makes you look more pro, but it would also make your code more readable and potentially more performant if you want to switch to native
https://create.roblox.com/docs/luau/type-checking

i think this variable name makes no sense; it should be like RandomGenerator

local NewRandom = Random.new()

its not clear that these are instances (likely folders), and they might be confused for arrays of particles/highlights. I would name it something like ParticleHolder

local Particles = game.ReplicatedStorage.Effects.Particles
local Highlights = game.ReplicatedStorage.Effects.Highlights
local Parts = game.ReplicatedStorage.Effects.Parts

This introduces way too much nesting; you should use guard clauses

-- nesting makes more unreadable
if TagInstance:FindFirstChild("Humanoid") then
		if not TagInstance:HasTag("ImmuneToFire") and not TagInstance:HasTag("CannotCatchFire") then

-- instead use guard cluase
if not TagInstance:FindFirstChild("Humanoid") then return end
if TagInstance:HadTag("ImmuneToFire") or TagInstance:HasTag("CannotCatchFire") then return end

I think this is way too much repetition
Just define a variable as TagInstance:FindFirstChild() and then write
if (obj) then dosomething(obj) end

	if TagInstance:FindFirstChild("Humanoid") then
		TagInstance.Humanoid:RemoveTag("BeingDamagedBySourceFire")
	end

	if TagInstance:FindFirstChild("Fire") then
		TagInstance:FindFirstChild("Fire"):Destroy()
	end
	
	if TagInstance:FindFirstChild("FireHighlight") then
		TweenService:Create(TagInstance:FindFirstChild("FireHighlight"), TweenInfo.new(1), {FillTransparency = 1, OutlineTransparency = 1}):Play()
	end

I think this one is the biggest flaw with the code
It’s much less readable and scalable to have anonymous functions inside events like this
You should generally put them as separate functions, but in this case since each one is correspondent to its own effect, you can put these functions inside a dictionary and then connect them in a for loop after

Then, if you want to add more effects, you can maintain readability by separating them into modules

CollectionService:GetInstanceAddedSignal("OnFire"):Connect(function(TagInstance)

Performance is fine and its still readable enough, but could be better

3 Likes
4

Overall it’s pretty good. I can read and understand it just fine which is the point. I could go through the code and maybe minorly nitpick things, but honestly almost any change I could even propose is just a waste of time to make probably as you could spend your time on other systems.

If I had to go and point out something, I would say the code may be slightly easier to follow if you broke up some of the larger functions into named functions (go too far and it ends up bad though). The other thing is this may be harder to extend than certain patterns would allow as there seems to be a lot of connection between the different types directly (like ice is directly removing fire immunity, but you may want fire to check if there is ice instead, or have a system that handles the affects that is external to this to promote a looser coupling) but that’s a very minor concern and only truly matters if this grows to the point it’s becoming a dependency pain to keep everything working because of interrelated status affects.

5

You should improve on this thing called separation of concerns basically too much code doing different things is bundled together its like a damage script also playing the animation part. To improve on this you can make effects data driven like:

Effect = {
    Tag = "OnFire",
    Duration = 10,
    DamagePerSecond = 5,
    Custom = FireEffect, --module
}
StatusManager:ApplyEffect(TagInstance, Effect)

but then again thats if you are planning for stacking effects or having a way to easily switch effects etc.

Another thing you could do its have several scripts listening on “OnFire” that each applies different things like one can handle the visuals the other handle statuses.

1 Like
6

Im starting to redo my approach to actual effects (modules instead of 1 big script) but I’ve for some reason never been a fan of having loads of scripts for one event. Surely having like 2 or 3 scripts listening for one event is just overdoing it right? I guess it depends whether I want readability or performance (unless I’m wrong)

1 Like
7

If you mean in the sense having multiple scripts read from an event independently, it’s generally best avoided unless the event is clearly global and meant to be referenced everywhere. You don’t want multiple scripts registering to an event because then if you ever need to change it, you’ll have to find all of the scripts that use it. If however the scripts are obviously going to use it, then this is less of an issue.

If you mean in the sense of we have 3 scripts to manage a single thing (not so much event related, but the concept itself) then it really just depends on style. As long as each script actually only does the thing it’s meant to it’s easy, but if they all are a random assortment of functions just use one script.

In terms of performance though, the amount of scripts you have mostly doesn’t matter. A script is effectively just a coroutine and a module script is effectively just a function with a return value that any script can access and share state with

1 Like
8

You’re presetting everything on top then using them to work with down the script.. Top-down.
That is not some lucky mistake you happen into. Shows you are skilled in your craft. I’m not even talking about what the script is or going to pick out anything that could be improved. You already are at a higher level than most, so great job. The rest will come in time..

1 Like
9

i made an effect system like this

I had one module acting as an effect service, and you can require it and do something like EffectService.ApplyEffect(player, "fire", 10 seconds)

the service handles adding/removing effects, then the actual code of what the effects do are put into separate modules under the service (1 module for 1 effect)

then you can compile those modules into a dictionary and at each tick do something like effects[currentEffect].tick()

and effects also have methods like onStart and onEnd for adding/removing visual effects for playing the effect

10

Bunch of bad practices.

  1. Use GetService always.
  2. Stop using the same member chain multiple times
local Particles = game.ReplicatedStorage.Effects.Particles
local Highlights = game.ReplicatedStorage.Effects.Highlights
local Parts = game.ReplicatedStorage.Effects.Parts

--// Changed to

local Particles = ReplicatedStorage.Effects.Particles
local Higthlights = ReplicatedStorage.Effects.Highlights
  1. Services should be declared above everything, since they serve as core.
  2. Use local functions. Always.
  3. Scrap away pairs, ipairs and every other iterator and use next or generalized iterator.
  4. Bad control flow variable naming i, v, they should be specific, and don’t override them with inner loop with the same exact name.
    image
    image127Ă—176 1.28 KB
  5. No words. Really.
    image
    image538Ă—168 5.89 KB
  6. The function is called SetPartSurfaceType not DelaySetPartSurfaceType, if you wan’t to yield before applying surfaces, do it manually before calling the function.
    image
    image599Ă—211 4.71 KB
11

just wanna say “No words. Really.” is not really constructive criticism? like just say what the issue is man, no need to be rude.

rest are fair though, ill have to learn about next as I’m not quite sure about it but ill try out the rest

1 Like
12

Readability is the issue, you have so many conditions, few are negated with not, some are not, consider writing it into a validatePart function and seperate each condition into variable and use them.

1 Like
13

I feel you, bro. You can improve ikr, I’ve been through that, you’ve done an impressive thing

1 Like