Need feedback on my 3 modules and script

Help and Feedback Code Review
#1

Script that calls the module:

local ServerStorage = game:GetService("ServerStorage")
local Players = game:GetService("Players")
local RunService = game:GetService("RunService")

local DetectModule = require(script.Detect)
local BulletModule = require(script.Bullet)
local MovementModule = require(script.Movement)

local TurrentsFolder = workspace.Turrents:GetChildren()
	
for _, turrent in ipairs(TurrentsFolder) do
		
	if turrent then
			
		while wait(1) do
				
			local target = DetectModule.Detect()
				
			if target then
				BulletModule.shootBullet(ServerStorage.Bullet, false, false, turrent.Main.Gun, 50, target,  workspace.Bullets)
				MovementModule.moveTurrent(turrent.Main.Gun, target)
			end
		end
	end
end

Module for moving the turrent:

llocal RunService = game:GetService("RunService")

local module = {}

function module.moveTurrent(turrent, target)
	
	 RunService.Heartbeat:Connect(function()
		turrent.CFrame = CFrame.new(turrent.Position, target.Position)
	end)
end


return module

Module for detecting player:

local RunService = game:GetService("RunService")
local Players = game:GetService("Players")

local target = nil

local module = {}

function module.Detect()
	
if #Players:GetPlayers() <= 0 then
	
    repeat wait()
			
    print("Checking Players")
			
	until #Players:GetPlayers() >= 1	
		
end
	

for _, player in ipairs(Players:GetPlayers()) do

		 local character = player.Character
						
		   if character then
						
			  local humanoid = character:FindFirstChildWhichIsA("Humanoid")
			  local humanoidRootPart = character:FindFirstChild("HumanoidRootPart")
							
			   if humanoid and humanoidRootPart then
					
				if humanoid.Health > 10 then
					if not target then
						target = humanoidRootPart
					end
				else
					target = nil
				end
			
				return target
			end
		else
			target = nil
		end
	end
end

return module

Module for shooting bullets:

local module = {}

function module.createBullet(bullet, anchored, canCollide, object, velocity, target, parent)
	
	local newBullet = bullet:Clone()
	newBullet.Anchored = anchored
	newBullet.CanCollide = canCollide
	newBullet.CFrame = object.CFrame
	newBullet.Parent = parent
		
	local newVelocity = Instance.new("BodyVelocity")
	newVelocity.Parent = newBullet
	
	newBullet.BodyVelocity.Velocity = object.CFrame.LookVector * velocity
end

function module.shootBullet(bullet, anchored, canCollide, object, target, velocity, parent)
	module.createBullet(bullet, anchored, canCollide, object, target, velocity, parent)
end

return module

I’m all ears on how I can improve the readability of code and it’s efficiency. :slight_smile:

3 Likes
#2

For this section of the script that calls the module, would it be searching for a new target every single second?

1 Like
#3

While I don’t follow the Unix’s philosophy of “one file does one thing very well”, you appear to be doing that.

I don’t understand why you are returning a table for each of the modules. You can return the functions themselves instead, so less memory is used and it is much clearer what is occurring. For the Bullet module, I’d return shootBullet only.

For the detection, don’t make it do more than one thing. You are detecting for when players are available to aim at. Then, you are targeting a character with a health condition. Throw away the part it detects players, that should be in your main script. That function should take an argument for the list of available players in lieu of that segment of code.

On your main script, use RunService.Heartbeat instead of wait(1). That is much more reliable than wait. I think that’s all I can criticize about. Otherwise, your scripts are well-made.

#4

One note:
If the bullet model isn’t too complex, it is faster to create new instances than to clone old ones

The difference is milliseconds but, if you’re wanting efficiency then…

2 Likes
#5

Do you test your scripts before posting here? Because this seems like it shouldn’t even work to begin with. If you need help with making your scripts work, post in #help-and-feedback:scripting-support instead.

If there’s more than one turr-et (1), only the first one will actually enter this loop, while all the others will wait for it to finish the loop, which never happens.

This never does anything. turrent is always something, because you just got it from the TurrentsFolder variable. There’s no time between the execution of that if statement and the GetChildrenCall(), so none of the children can have been destroyed.

You call this once every second for each turret (or for the first one, anyway, because of the bug I mentioned earlier). That means you’re setting up a Connection to RunService.Heartbeat every second. That means after 10 seconds, there’s 10 different Connections all trying to move the turret to a target, which may or may not be the same for each connection. Unfortunately it might actually work, because the last connection made probably gets called last, so the turret moves to the wrong target 9 times and then moves to the last target that was set. It’s bad that it works though, because you won’t notice that anything is wrong until your game starts lagging because of the 10000 updates that are done each Heartbeat for no reason.

TL;DR: Don’t set up new connections without disconnecting old ones that should no longer do anything.

Can never be less than 0, just use == to better show what you actually want the check to represent. The print statement is confusing, you’re not “checking players”. You’re “waiting for players”. You can replace that whole if statement with a loop inside with just a while loop:

    while #Players:GetPlayers() == 0 do
        print("Waiting for players")
    end

module.shootBullet is completely pointless, it just makes it more confusing how to use the bullet module (which TBH shouldn’t exist in the first place). If you reeeeally wanted to do it the way you’ve done it, you can use varargs (...) to not have to type every argument in both the function header and the call to createBullet:

function module.shootBullet(...)
    return module.createBullet(...)
end

The detect function should probably just return nil instead of waiting for players to join / respawn.

[1]: You can find- and- replace every occurrence of a string pattern in a script by pressing Ctrl+H.

Let me know if you have any questions

1 Like
#6

The scripts work perfectly fine. No problem at all, I of course test and debug them.

1 Like
#7

Please don’t edit the code @SilentsReplacement you provided as continuity is lost (someone suggest change → edit to change; now the suggestion is out-of-context).

1 Like