Any better way to write this?

Help and Feedback Code Review
1

IS there a better way to write this so it’s neater and easier to read / more secure etc?

The idea is it takes the player and a ‘actionContext’ which is a string either M1 or M2 then determines what tool you have and which function to fire / with what information.

game.ReplicatedStorage.Events._useTool.OnServerEvent:Connect(function(player, actionContext)
	-- Determine what the 'player' is trying to do here.
	
	local character = player.Character
	local selectedTool
	
	-- first lets determine what tool you're trying to use :)
	
	if character:FindFirstChildOfClass('Tool') then
		selectedTool = character:FindFirstChildOfClass('Tool')
	else
			--// PUNCH LOGIC \\--
		if actionContext == 'M1' then
			combatModule.MouseButton1_Attack()
		end
		
		if actionContext == 'M2' then
			combatModule.MouseButton2_Attack()
		end
	end
	
	if selectedTool then
		
		-- determine what to do!! :)
		
		if selectedTool:GetAttribute('toolType') == 'weapon' then
			if actionContext == 'M1' then
				combatModule.MouseButton1_Attack()
			end

			if actionContext == 'M2' then
				combatModule.MouseButton2_Attack()
			end
		end
		
		if selectedTool:GetAttribute('toolType') == 'ability' and actionContext == 'M1' then
			warn('no logic for abilities yet :( come back later')
		end
		
		if selectedTool:GetAttribute('toolType') == 'food' then
			warn('no logic for food!')
		end
		
		if selectedTool:GetAttribute('toolType') == 'potion' then
			warn('no logic for potions either!')
		end
		
		if selectedTool:GetAttribute('toolType') == 'artifact' then
			warn('no logic for artifiacts yet c:')
		end
		
		-- more logic here
	end
end)
2 Likes
2

I would suggest moving game.ReplicatedStorage as a variable above like RS or smth. Then I would recommend adding types to all your variables like players and selectedTool. To make it more secure maybe check if the humanoid is alive or the character isnt nil just in case the player uses the tool when dead. Other then that its pretty readable

3

Don’t use conditions for this.
Use a toolType map for it.

local toolTypeMap = {
    food = function(player: Player, tool: Tool): ()
        warn 'No logic for food yet!'
    end,

    ...

}


local callback = toolTypeMap[selectedTool:Get attribute('toolType')]
if callback then callback(player, selectedTool) end

Also I suggest you to create your own Enum for Tool Types.

4

This a bad practice as RS isn’t a very helpful variable name. RS can mean plenty of things - RunService, ReplicatedStorage, etc. I suggest to keep variable names short but descriptive. Though I also recommend moving game.ReplicatedStorage as a variable.