How can I improve my code?

Help and Feedback Scripting Support
1

So, I have attempted making large open world survival games numerous times over the past years, and well, they always end up turning into a huge mess of code that is hard to read and hard to build off of (somewhat unavoidably considering the sheer volume of game logic associated with building such a game). This is obviously because I am still not a very good programmer (and not the best problem solver), or at least not good enough to write clean code that other programmers can understand and that I can come back to and figure out quickly to continue building off of it.

Anyways, I have decided to start from the ground up again, programing the inventory system for my game first. Is there any way I can Refactor/Reformat/Rewrite any of this code to be cleaner, easier to interpret and more performance efficient?

print (script.Name.." Loaded.");

local Players = game:GetService("Players");
local UserInputServiceService = game:GetService("UserInputService");
local ReplicatedStorage = game:GetService("ReplicatedStorage");
local RunService = game:GetService("RunService");

local Player = Players.LocalPlayer
local PlayerGui = Player:WaitForChild("PlayerGui");
local PlayerMouse = Player:GetMouse()

local PrimaryInterface = PlayerGui:WaitForChild("PrimaryInterface");
local CursorDisplay = PrimaryInterface:WaitForChild("CursorDisplay")
local Inventory = PrimaryInterface:WaitForChild("Inventory");
local Items = Inventory:WaitForChild("Items");
local PrimaryInterfaceData = PrimaryInterface:WaitForChild("Data");
local UIClones = PrimaryInterfaceData:WaitForChild("UIClones")
local ToggleInventoryButton = PrimaryInterface:WaitForChild("ToggleButton");
local DescriptionDisplay = Inventory.Items:WaitForChild("DescriptionDisplay");
local ItemsHeldInInventory = PrimaryInterfaceData.InventoryData.ItemsHeldInInventory;

local Modules = ReplicatedStorage:WaitForChild("Modules");
local FunctionLibrary = require(Modules:WaitForChild("FunctionLibrary"))



local function AddToItemStack(Slot, SlotData, ItemData)
	SlotData.Quantity.Value = SlotData.Quantity.Value + ItemData.Quantity.Value;
	Slot.StackQuantityDisplay.Text = "x"..SlotData.Quantity.Value;
	FunctionLibrary.DestroyObject(ItemData.Parent);
	end



local function AddItemToSlot(Slot, SlotData, ItemData)
	ItemsHeldInInventory.Value = ItemsHeldInInventory.Value + 1;
	SlotData.ImageModel.Value = ItemData.ImageModel.Value;
	SlotData.ItemName.Value = ItemData.ItemName.Value;
	SlotData.ItemType.Value = ItemData.ItemType.Value;
	SlotData.Nutrition.Value = ItemData.Nutrition.Value;
	SlotData.Quantity.Value = ItemData.Quantity.Value;
	SlotData.SlotHasItem.Value = true;
	if ItemData.Stackable.Value == true 
		then
		Slot.StackQuantityDisplay.Text = "x"..SlotData.Quantity.Value;
		Slot.StackQuantityDisplay.Visible = true;
	end
	local ImageModelClone = ItemData.ImageModel:Clone()
	ImageModelClone.Parent = Slot.Viewport; 
	FunctionLibrary.DestroyObject(ItemData.Parent);
end



local function CheckForSpaceInInventory(ItemData)
	local Slot = Items:FindFirstChild("Slot"..ItemsHeldInInventory.Value + 1);
	if Slot and Slot.SlotData.SlotHasItem.Value == false then
		AddItemToSlot(Slot, Slot.SlotData, ItemData);
	else
		print("Not enough space in inventory");
	end
end



local function CheckIfItemStacks(ItemData)
	local FoundExistingStack  = false;
	if ItemData.Stackable.Value == true then
		for _, Slot in ipairs(Items:GetChildren()) do
			if Slot:IsA("ImageButton") 
				and Slot.SlotData.ItemName.Value == ItemData.ItemName.Value
				and Slot.SlotData.SlotHasItem.Value == true
			then
				FoundExistingStack = true;
				AddToItemStack(Slot, Slot.SlotData, ItemData);
			else 
				-- check for space to store new item
				CheckForSpaceInInventory(ItemData);
			end
			break
		end
	else 
		if ItemData.Stackable.Value == false then
			CheckForSpaceInInventory(ItemData);
		end
	end
end



local function CheckItemType(ItemData)
	if ItemData.ItemType.Value ~= "Tool" then
		CheckIfItemStacks(ItemData)
	end
end



local function PlayerClickedItem(Input)
	if Input.UserInputType == Enum.UserInputType.MouseButton1 
	or Input.UserInputType == Enum.UserInputType.Touch 
	then
		if PlayerMouse.Target then
			local ItemData = PlayerMouse.Target:FindFirstChild("Item") 
			or PlayerMouse.Target.Parent:FindFirstChild("Item")
			if ItemData then
				CheckItemType(ItemData);
			end
		end
	end
end
UserInputServiceService.InputBegan:Connect(PlayerClickedItem);



local function MouseHoverOverItem()
	if PlayerMouse.Target
		and PlayerMouse.Target:FindFirstChild("Item") 
		or PlayerMouse.Target 
		and PlayerMouse.Target.Parent:FindFirstChild("Item") 
	then
		local ItemData = PlayerMouse.Target:FindFirstChild("Item") or PlayerMouse.Target.Parent:FindFirstChild("Item");
		CursorDisplay.Position = UDim2.new(0, PlayerMouse.X + 25, 0, PlayerMouse.Y);
		CursorDisplay.Text = ItemData:FindFirstChild("ItemName").Value;
		CursorDisplay.Visible = true;
	else 
		CursorDisplay.Visible = false;
	end
end
PlayerMouse.Move:Connect(MouseHoverOverItem)



local function ToggleInventoryVisibility()
		if Inventory.Visible == false then
		Inventory.Visible = true
	else if Inventory.Visible == true then
			Inventory.Visible = false
			Inventory.UIClones:ClearAllChildren()
			-- remove ui menus 
		end
	end
end
ToggleInventoryButton.MouseButton1Click:Connect(ToggleInventoryVisibility)



local function ToggleItemDescriptionDisplay(ItemName, Show)
	if Show == true then
		DescriptionDisplay.Visible = true;
		DescriptionDisplay.Text = ItemName.Value;
	else 
		DescriptionDisplay.Visible = false;
	end
end



function ShowItemMenu(Slot, SlotData)
	local UIClone = UIClones:FindFirstChild(SlotData.ItemType.Value.."UI"):Clone()
	UIClone.Parent = Inventory.UIClones;
	UIClone.Position = UDim2.new(0, PlayerMouse.X, 0, PlayerMouse.Y);
	UIClone.Visible = true;
	UIClone.SlotLink.Value = Slot;
end



local function InventoryInteractionController()
	for _, v in ipairs(Inventory.Items:GetChildren()) do
		if v:IsA("ImageButton") 
			then
		v.MouseEnter:Connect(function()
			local SlotData = v.SlotData;
			if SlotData.SlotHasItem.Value == true then
					ToggleItemDescriptionDisplay(SlotData.ItemName, true)
					v.MouseLeave:Connect(function()
						DescriptionDisplay.Visible = false;
					end)
				else
					ToggleItemDescriptionDisplay(SlotData.ItemName, false)
				end
			end)
		v.Activated:Connect(function()
				local SlotData = v.SlotData;
				if v.SlotData.SlotHasItem.Value == true
				--if slot has an item
				then
					Inventory.UIClones:ClearAllChildren()
					--remove other possible menus
					ShowItemMenu(v, SlotData)
				end
			end)
		end
	end
end
InventoryInteractionController()
1 Like
2

I noticed a few things that you could maybe benefit from.

//–//

When checking the condition of a boolean in an if statement, it is not necessary to compare it to true or false. Consider the example:

local T = true
local F = false

if T then
    -- T is a true statement
end
if F then
    -- F is not a true statement
end
if not F then
    -- Not (opposite of) F is true
end
Reason this works (if interested)

In an if statement, its only job is to check if the provided statement is equal to true. Think of an invisible == true at the end of every statement. Here is an example with a Roblox function:

if Slot:IsA("ImageButton") then

The :IsA() function above returned true, and since true is equal to true, Lua accepts the statement. Consider an example using false statements.

local Var = false
if Var then -- does not accept
end
if not Var then --accepts
end

//–//

Second, some operators can be simplified. When you want to add onto a preexistent value, you do not need to rewrite the value. That is hard to put into words, so consider the example:

local x = 1
-- Long, unneccessary way:
x = x + 1
-- Better way:
x += 1

The above example also applies to division (/=), multiplication (*=), subtraction (-=) and modulo (%=).

//–//

Sometimes, you don’t need to use if statements, and could resort to a much simpler method if you need to make a simple conditional statement.
Method: (Value) = (condition) and (if true) or (if false)
Consider the example:

local Var1 = 4
local Var2 = true
Type.Value = (Var1 < 5 and Var2) and Var1 or 5 --Type.Value is now equal to Var1 (4)
Var1 = (Type.Value == Var1) and (2 * Var1) or (Var1 - 1)

print(Type.Value)
print(Var1)

-- Output:
-- 4
-- 8

Otherwise, you put it together real nicely, and I don’t have many other comments.

1 Like
3

I haven’t read through the code, but I didn’t see enough comments. You should go through and add more comments in it so that you can remember what each code block does.

2 Likes
4

Consider using anonymous functions if you’re only going use a function on an specific event.

5

For the “if then” statements
Use this for checking false statements

if not variable then

Use this for checking true statements

if variable then

Not much of a change but helps ig?