Feedback on placement system

Help and Feedback Code Review
1 
local Players = game:GetService("Players")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local UserInputService = game:GetService("UserInputService")

local player = Players.LocalPlayer
local character = player.Character or player.CharacterAdded:Wait()
local mouse = player:GetMouse()

--> Constants

local TURRENT_PLACEMENT_START = Enum.KeyCode.E
local TURRENT_PLACEMENT_END = Enum.KeyCode.B 
local TURRENT_ROTATE = Enum.KeyCode.R
local ROTATION_OFFSET = 45

local isPlacing = false

--> Connections
local mouseConnection

local part = Instance.new("Part")
part.Anchored = true
part.Parent = workspace

--> Private Functions 

local function rotate(part) 
    if part:IsA("BasePart") then
        part.CFrame *= CFrame.Angles(0, math.rad(ROTATION_OFFSET), 0)
    else
        if part:IsA("Model") then
            if part.PrimaryPart then
                part.PrimaryPart.CFrame *= CFrame.Angles(0, math.rad(ROTATION_OFFSET), 0)
            end
        end
    end
end 

local function position(part)
    if (character.HumanoidRootPart.Position - mouse.Hit.Position).Magnitude > 20 then
        return
    end
    part.Position = mouse.Hit.Position 
end

UserInputService.InputBegan:Connect(function(input, processed)
    if processed then return end

    if input.KeyCode == TURRENT_PLACEMENT_START then
        mouse.TargetFilter = part
        mouseConnection = mouse.Move:Connect(function()
            position(part)
        end)

    elseif input.KeyCode == TURRENT_PLACEMENT_END then
        if mouseConnection then
            mouseConnection:Disconnect()
        end

    elseif input.KeyCode == TURRENT_ROTATE then
        if mouseConnection then 
            rotate(part)
        end
    end
end)

Note that this is not a grid placement system. I would like feedback on;

  • rotate function
  • position function
  • InputBegan event
3 Likes
2

It’s all really really good. Well done!

There’s always room for improvement though. Here’s all my nitpicks and stuff :stuck_out_tongue:

  • The rotate function handles two cases, one for the parameter being a Part and one for the parameter being a Model.
    • Change the parameter name to reflect this, e.g. to “instance” (same for he position function).
    • Why doesn’t the position function handle both cases like rotate?
  • In the rotate function, you could save 1 line of code by doing elseif instead of else if.
  • You could save another LoC by doing if (part:IsA("Model") and part.PrimaryPart) then
  • If the Model doesn’t have a PrimaryPart, is that a bug? If so, you should probably warn() or error() instead of silently failing. You’ll thank yourself when you save 30 minutes looking for a hard-to-find bug caused by this.
  • “position” is both a verb and a noun in English, so it’s confusing as a variable/function name. It’s usually assumed that the noun meaning is used, e.g. in BasePart.Position, so avoid using the verb meaning as function names for words like that. Call it “setPosition” instead, and change the rotation function to have the same style.
  • In the position function, you’re looking up mouse.Hit twice. Consider doing it only once and saving it in a variable. We can’t know ´cause we can’t read Roblox source code, but it might do a raycast each time to determine what mouse.Hit is.
    • We save a tiny bit of performance this way (not important, it would be a silly way to try and optimize, but good to be aware of).
    • Looking up mouse.Hit twice, thinking it’s going to be the same both times, can cause bugs. E.g. if you move or Destroy the hit part between the two lookup, it’s not going to be the same.
  • You misspelled “turret” (you’ve got an extra ‘N’).
  • In the InputBegan listener, you’re checking if mouseConnection to see if a turret is currently being placed. Use a variable called isPlacing (outermost scope, or local to the relevant scope) instead, because otherwise the reader has to guess why you’re doing that check. Alternatively forget the variable and put a comment there.
  • You’re defining isPlacing at the top of the script, but never set or use it.
  • I’d add “_KeyCode” to the end of the keycode constants, to make it even clearer what they are.
4 Likes