Feedback on my new module

DullXxxswagfoxyDev · April 9, 2021, 9:16am

Can I have some feedback on this module?

local BattleEssentials = {}

local Settings = {

	--Some quick Variables

	ModuleVersion = "1.0", --Defining the module's version. (Required for the "ModuleInformation" function)
	
	DefaultWalkSpeed = 16,
	DefaultJumpPower = 50,
	
	StunnedWalkSpeed = 6,
	StunnedJumpPower = 24,

	--This is making sure that the user didn't decide to delete the attributes. 
	--The module will run an error when is attempted to be used without attributes.

	IsBurnEnabled = script:GetAttribute("IsBurnEnabled"),
	IsFreezeEnabled = script:GetAttribute("IsFreezeEnabled"),
	IsForceFieldEnabled = script:GetAttribute("IsForcefieldEnabled"),
	IsStunEnabled = script:GetAttribute("IsStunEnabled"),
	IsHealEnabled = script:GetAttribute("IsHealEnabled")


}

-- Functions (This is where all the magic happens!)

function BattleEssentials.ModuleInformation()
	local String = "BattleEssentials Module Version ".. Settings.ModuleVersion .." \n									Made by DullXxxswagfoxyxxX"
	print(String)
end

function BattleEssentials.Freeze(PlayerCharacter, Duration)
	if Settings.IsFreezeEnabled == true then 
		print("Function Authorised")
		
		local Humanoid = PlayerCharacter:FindFirstChild("Humanoid")
		local Particle = script.Particles.FreezeParticle:Clone()
		
		Humanoid.WalkSpeed = 0
		Humanoid.JumpPower = 0
		Particle.Parent = PlayerCharacter.Torso
		
		wait(Duration)
		
		Particle:Destroy()
		Humanoid.WalkSpeed = Settings.DefaultWalkSpeed
		Humanoid.JumpPower = Settings.DefaultJumpPower
	else
		error("Function was blocked by developer, if this is not meant to happen please check your module attributes")
	end
end

function BattleEssentials.Stun(PlayerCharacter, Duration)
	if Settings.IsStunEnabled == true then
		print("Function Authorised")
		
		local Humanoid = PlayerCharacter:FindFirstChild("Humanoid")
		local Particle = script.Particles.StunParticle:Clone()
		
		Particle.Parent = PlayerCharacter.Torso
		Humanoid.WalkSpeed = Settings.StunnedWalkSpeed
		Humanoid.JumpPower = Settings.StunnedJumpPower
		
		wait(Duration)
		
		Particle:Destroy()
		Humanoid.WalkSpeed = Settings.DefaultWalkSpeed
		Humanoid.JumpPower = Settings.DefaultJumpPower
	else
		error("Function was blocked by developer, if this is not meant to happen please check your module attributes")
	end
end

function BattleEssentials.Burn(PlayerCharacter, Duration, Damage)
	if Settings.IsBurnEnabled == true then
		print("Function Authorised")
		
		local Humanoid = PlayerCharacter:FindFirstChild("Humanoid")
		local Particle = script.Particles.FireParticle:Clone()
		local FireDuration = Instance.new("IntValue")
		
		FireDuration.Name = "FireDuration"
		FireDuration.Value = Duration
		FireDuration.Parent = PlayerCharacter
		
		Particle.Parent = PlayerCharacter.Torso
		
		while FireDuration.Value > 0 do
			wait(1)
			
			Humanoid:TakeDamage(Damage)
			FireDuration.Value = FireDuration.Value - 1
		end
		
		Particle:Destroy()
	else
		error("Function was blocked by developer, if this is not meant to happen please check your module attributes")
	end
end

function BattleEssentials.Heal(PlayerCharacter, Ammount)
	if Settings.IsHealEnabled == true then
		print("Function Authorised")
		
		local Humanoid = PlayerCharacter:FindFirstChild("Humanoid")
		local Particle = script.Particles.HealParticle:Clone()
		
		Humanoid.Health = Humanoid.Health + Ammount
		
	else
		error("Function was blocked by developer, if this is not meant to happen please check your module attributes")
	end
end

function BattleEssentials.MaxHeal(PlayerCharacter)
	if Settings.IsHealEnabled == true then
		print("Function Authorised")

		local Humanoid = PlayerCharacter:FindFirstChild("Humanoid")
		local Particle = script.Particles.HealParticle:Clone()

		repeat wait() Humanoid.Health = Humanoid.Health + 1 until Humanoid.Health == Humanoid.MaxHealth

	else
		error("Function was blocked by developer, if this is not meant to happen please check your module attributes")
	end
end


return BattleEssentials

The function names pretty much explain what they do.

EmbatTheHybrid · April 9, 2021, 10:17am

There’s only a real few things that I noticed

I think it would better if you allow users of your module, if you ever make it public, to be able to change the Is(thing)Enabled settings from the module itself rather than changing an attribute, which would organize it a little so everything can be changed from the module itself rather than some fro mthe module, and some from attributes

In your Fire damage function, it would be better to use a numerical for loop rather than making an intvalue to damage for a certain amount of time, so your fire damage function can be turned into

function BattleEssentials.Burn(PlayerCharacter, Duration, Damage)
	if Settings.IsBurnEnabled then
		print("Function Authorised")
		
		local Humanoid = PlayerCharacter:FindFirstChild("Humanoid")
		local Particle = script.Particles.FireParticle:Clone()
		
		Particle.Parent = PlayerCharacter.Torso
		
		for i = Duration, 0, -1 do
			wait(1)
			Humanoid:TakeDamage(Damage)
		end
		
		Particle:Destroy()
	else
		error("Function was blocked by developer, if this is not meant to happen please check your module attributes")
	end
end

Unless you actually need that intvalue in which case keep it as before, just reemmber to destroy the IntValue as in your code, it is not destroyed once the loop ends

I recommend you use compound assignments rather than how you were doing to subtract something from a value. Example instead of

Humanoid.Health = Humanoid.Health + 1

Do

Humanoid.Heatlh += 1

It helps simplify the code and makes it look nice imo, especially if you have a long reference. There are other compound assignments such as -= *=, /= which work how you’d expect them to.

Also I see in your healing functions, you clone a Heal particle but never parent it to anything, such as the player’s torso, it’s just left there.

Also IsForceFieldEnabled doesn’t seem to do anything, but I presume it’s going to be added later

That’s all I can really see as of now, besides that it, I don’t really see any other problems

DullXxxswagfoxyDev · April 9, 2021, 10:29am

Thanks for the feedback. I chose attributes for a much easier way of toggling different things.

EmbatTheHybrid · April 9, 2021, 10:32am

If you’re fine it being with attributes, then there’s no harm in that. I mostly suggested that you allow it to be changed from the module instead since it would be simpler to change settings if they’re located in the same place rather than some in one place and others in other places, just another way of adding some more organization

xZylter · April 9, 2021, 2:45pm

Use error() sparingly. In most cases using error() isn’t a good choice as it terminates the last protected function.

I would highly recommend you change it out for warn() as this will ensure your module can still be used. Especially in this scenario where there is no real consequences of the condition not being met.

DullXxxswagfoxyDev · April 11, 2021, 2:49am

Could you elaborate on what you mean by this?

xZylter · April 11, 2021, 3:27am

On Roblox each script is kept contained within itself, if at any point in the script an error is found that script, and only that script, will stop running. So in the case of your module, using error() would make the module it self stop running and in effect the script that called the function. Because in the backend, it’s the script that required the module that is running the code.

Really this isn’t advised as it puts your game in a state where code will not work as an error is seen as an irrecoverable state for the script. If you called any of the functions without the corresponding setting value is set to false this module will no longer work in that server. Which can have serious impacts on your game when what seems to be the main combat mechanics in your game don’t do anything.

You can use pcalls to add protected calls within each script but it is not relevant to this as you can avoid the issue by just using warn() instead.

utrain · April 11, 2021, 4:59am

Don’t wait on me!
In a lot of your code, you yield with wait(). That itself is a problem because wait() can become very imprecise as a function. But, there’s another problem, you’re yielding on the main corountine. Meaning, whenever a user of this module calls let’s said, Freeze, the duration the player is frozen for is also the same amount of time the main corountine is effectively yielded for.

The solution? Use RunService’s heartbeat event instead. We’ll listen on that event and it won’t block main corountine. It passes the time elapsed between the last and the current frame. An example for your .Burn. We’ll have to keep track of whenever one second has elapsed to damage the player. Also, we’ll keep track of the fire duration as well.

-- ... Skipping code ...
local AccumulatedTime = 0
local HeartbeatConnection

HeartbeatConnect = Runservice.Heartbeat:Connect(
    function(Delta)
        AccumulatedTime += Delta

        if AccumulatedTime >= 1 then
            Humanoid:TakeDamage(Damage)
            FireDuration.Value -= AccumulatedTime
            AccumulatedTime = 0
        end

        if FireDuration.Value < 0 then
            -- Cleaning up our mess...
            HeartbeatConnect:Disconnect()
            Particle:Destroy()
        end
    end
)
-- ... Let's skip the end too ...

I’ll let you do the rest of them.

Why attributes?
There’s absolutely no need for attributes that booleans can’t do. That is just a layer of unneeded misdirection. Why should it even matter anyways? Do this instead (less repeating anyways)…

local Setting = {
	-- ...
	IsBurnEnabled = true,
	IsFreezeEnabled = true,
	IsForceFieldEnabled = true,
	IsStunEnabled = true,
	IsHealEnabled = false -- Haha, I'm evil!
}

Make sure you’re being DRY!
DRY means don’t repeat yourself! You repeat yourself a lot with this code structure:

local function AnAction()
     if Settings.IsAnActionEnabled == true then
          print("Function Authorized")
          -- Do that something...
     else
		error("Function was blocked by developer, if this is not meant to happen please check your module attributes")
	end
end

Firstly, let’s get rid of the print function you have. It serves no purpose as it doesn’t specify what was authorized. Additionally, it is non-critical debugging.

Before I forget, taking @xZylter’s advice, we’ll make the error into a warning instead.

Thirdly, let’s make a new function called BlockedError. We’ll make sure to include information what function was invoked!

local BLOCKED_MESSAGE = "Function %s blocked by developer, if this is not meant to happen please check your module attributes" -- A constant...

local BlockedWarning = function(Name)
     warn(BLOCKED_MESSAGE:format(Name))
end

Alright, now we can just define each function as either the BlockedError or the actual function! Like so…

-- ...
if Settings.IsFreezeEnabled then
	function BattleEssentials.Freeze(PlayerCharacter, Duration)
		local Humanoid = PlayerCharacter:FindFirstChild("Humanoid")
		local Particle = script.Particles.FreezeParticle:Clone()
		-- ...
	end
else
	function BattleEssentials.Freeze()
		BlockedWarning("Freeze")
	end
end
-- ...