How can I improve my Shop Gui

Help and Feedback Code Review
#1

So right now the code it bassically clones a template of a gui button and I use a dictionary to fill in the remaining data and I’m not sure if using a bunch of else ifs are really that good

I want to see if there are any other ideas

--Services-- 
local rs = game:GetService("ReplicatedStorage")
local Players = game:GetService("Players")

--Variables-- 


local modulesFolder = rs:WaitForChild("modules")
local shopcontents = require(modulesFolder.shopcontents)
local itemTypes = shopcontents:getItemTypes()


local player = Players.LocalPlayer
local gui = player.PlayerGui 
local shopGui = gui.ShopGui 
local scrollingFrame = shopGui.ScrollingFrame
local template = scrollingFrame.Template
local buy = template.Buy 
local imageButton = template.ImageButton

-- Loops -- 

for i = 1, shopcontents.GuiSlots do 
	local GuiBox = template:Clone()
	GuiBox.Name = "GuiBox"..i
	GuiBox.Parent = scrollingFrame
end


for i,v in pairs(scrollingFrame:GetChildren()) do 
	if v.Name == "GuiBox"..1 then 
		v.Buy.Text = itemTypes.Item1.Cost
		local imagenum = itemTypes.Item1.ImageId 
		v.ImageButton.Image = "rbxassetid://"..imagenum 
		
	else if v.Name == "GuiBox".. 2 then 
			v.Buy.Text = itemTypes.Item2.Cost 
			local imagenum2 = itemTypes.Item2.ImageId 
			v.ImageButton.Image = "rbxassetid://"..imagenum2 
			
		end
	end
end


local shopContents = {}

shopContents["GuiSlots"] = 2

function shopContents:getItemTypes()
	local itemTypes = { 
		["Item1"] = {
			["Cost"] = 100; 
			["ImageId"] = 904635286;
			
			
		};
		["Item2"] = { 
			["Cost"] = 125; 
			["ImageId"] = 112902314
			
			
		};
		
		
	}
	
	return itemTypes 
end 

	


return shopContents
1 Like
#2

Hey there @n_cy! Great start so far, but remember to always keep your code as dynamic as possible! here’s a possible implementation for what you seek for (although I changed the naming scheme to fit more with my own self-prescribed style, so don’t assume copy + pasting will work).

I like to disclaim before every time I rewrite someone’s code in its entirety that this is not a prescriptive solution, rather, it’s a resource to help better inform your style, organization, and process when designing things like this.

--[[
    @typedef {table} ShopItem - Represents an entry in the shop.
    @property {number} cost - How much the item costs.
    @property {string} displayName - The stylized name of the item.
    @property {number} imageId - The asset id of the corresponding item.
]]

--[[
    This could be an array of objects instead of a dictionary, but the id makes
    it more accessible for other non-gui tasks.

    @type {table<string, ShopItem>}
]]
return {
    item1 = {
        cost = 100,
        displayName = "Item 1",
        imageId = 904635286,
    },
    item2 = {
        cost = 125,
        displayName = "Item 2",
        imageId = 112902314,
    },
};


local Players = game:GetService("Players");
local ReplicatedStorage = game:GetService("ReplicatedStorage");

local itemData = require(ReplicatedStorage:WaitForChild("Modules").ShopItemData);
local shopScrollingFrame = Players.LocalPlayer.ShopGui.ScrollingFrame;

--[[
    Creates a new gui element to be displayed in the local player's shop gui.

    @param {ShopItem} data - The shop item to be displayed by this element.
    @returns {GuiObject} - (not sure what the template is).
]]
local function createShopGuiElement(data)
    local shopGuiElement = shopScrollingFrame.Template:Clone();
    -- This isn't very ergonomical to any developers but shouldn't matter as the
    -- reference to the instance should be communicated, not indexed by other
    -- scripts since the content can't be assumed.
    shopGuiElement.Name = data.displayName;
    shopGuiElement.Parent = shopScrollingFrame;

    shopGuiElement.PriceElement.Text = data.cost;
    shopGuiElement.ImageButton.Image = ("rbxassetid://%d"):format(data.imageId);

    -- TODO: Display the name of the item to the user.

    return shopGuiElement;
end

--[[
    Reads the item data for the shop and populates the gui.

    @returns {nil}
]]
local function populateShopGui()
    -- No need for us to use the easier-to-interact-with item id, so we use the
    -- `_` placeholder so our script analysis (linter) doesn't complain about an
    -- unused variable.
    for _, data in pairs(itemData) do
        local shopGuiElement = createShopGuiElement(data);
    end
end

populateShopGui();

As always, if you have any questions as to how or why something is the way it is, feel free to ask!

2 Likes
#3

Hi I made some changes with your advice, I changed the dictionary into a table and I have started using _ in my in pairs loop when i don’t need the i. I have one question though, what would be a better way instead of cloning?

#4

I really don’t see an issue with using a template. The performance impact outweighs the developer ergonomics of creating it on-site.

Also, it’s easier to maintain a template than to hardcode the way the gui should look.

1 Like