Functions that manipulate player data, any better way?

Help and Feedback Code Review
#1

my code adds items to a player’s inventory, and I find the way I’m currently doing it to feel really messy
(fetches inventory > updates cache (datastore2 saving))

OLD CODE

the way im currently doing it

local Players = game:GetService("Players")
local Quire = _G.Quire
local DataStore2 = Quire("DataStore2")
local Enums = Quire("Enums")
local NetworkUtil = Quire("NetworkUtil")
local ItemInfos = Quire("ItemInfos")
local SessionData = Quire("SessionData")
local ItemFunctions = Quire("ItemFunctions")
local repr = require(3148021300)

local INVENTORY_KEY = Enums.DataKeys.InventoryKey
local DEFAULT_INVENTORY = {
    Slots = {
        [1] = {
            IsHotSlot = true,
            Item = nil
        },
        [2] = {
            IsHotSlot = true,
            Item = nil
        },
        [3] = {
            Item = nil
        },
        [4] = {
            Item = nil
        },
        [5] = {
            Item = nil
        },
        [6] = {
            Item = nil
        },
        [7] = {
            Item = nil
        },
        [8] = {
            Item = nil
        },
    }
}



local InventoryService = {}

local function loadPlayerInventory(player)
    local inventory = DataStore2(INVENTORY_KEY, player):GetTable(DEFAULT_INVENTORY)

    for _, slotData in ipairs(inventory.Slots) do
        local item = slotData.Item
        if item then
            ItemFunctions.SetItemInfo(item)
        end
    end

    InventoryService:AddItem(player, 1, 15)
    
end

function InventoryService:AddItem(player, ID, amount)
    DataStore2(INVENTORY_KEY, player):Update(function(inventory)
        amount = amount or 1

        local itemInfo = ItemInfos:GetInfoFromID(ID)
        assert(itemInfo, "Invalid Item")

        local inventorySlots = inventory.Slots

        local function transferAmountToItem(item, amountToTransfer)
            ItemFunctions.IncreaseItemStackSize(item, amountToTransfer)
            amount = amount - amountToTransfer
        end

        (function()
            -- adds amountToAdd to open stacks of same id
            if ItemFunctions.IsItemStackable(itemInfo) then
                for slotIndex, slotData in ipairs(inventorySlots) do
                    local item = slotData.Item
                    local itemIsID = item and ItemFunctions.IsItemID(item, ID)
                    local amountToTransfer = itemIsID and math.clamp(amount, 0, ItemFunctions.GetOpenStackSize(item))
    
                    if amountToTransfer then
                        transferAmountToItem(item, amountToTransfer)
                        if amount == 0 then return end
                    end
                end
            end

            -- adds items to empty slots
            for slotIndex, slotData in ipairs(inventorySlots) do
                if not slotData.Item then
                    local item = {ItemInfo = itemInfo, ID = ID, StackSize = 0}
                    transferAmountToItem(item, math.clamp(amount, 0, ItemFunctions.GetOpenStackSize(item)))
                    slotData.Item = item
                    if amount == 0 then return end
                end
            end
        end)()
        
        print(repr(inventory, {pretty = true}))

        return inventory
    end)
    return amount
end

Players.PlayerAdded:Connect(loadPlayerInventory)

return InventoryService

the main part I’m finding messy is the getting/saving data with datastore2 for every function that is directly called and manipulates player data

Newer Code

Each player has a inventoryCache, the inventory has slots which always exist, and each slot can have an item, each item has an ID, StackSize, and a metatable linking to a iteminfo table (MaxStackSize, ItemClasses, Damage, etc)

in my previous code I was passing the player as a parameter and using Datastore:Update() to get the cache for every method and save it

now what I am doing is calling methods with a inventoryCache parameter instead of player, and it has a metatable which allows me to get the owner of a inventoryCache by doing inventoryCache.Owner which allows me to not have to pass the player as another parameter

I was hesitating a bit to do a cache system on top of the cache system datastore2 already has, but doing so has allowed me to make more use of metatables, and looks cleaner

e.g

InventoryService:AddItem(inventoryCache, ID, amount)
Inventory Service 
local Players = game:GetService("Players")
local Quire = _G.Quire
local DataStore2 = Quire("DataStore2")
local Enums = Quire("Enums")
local NetworkUtil = Quire("NetworkUtil")
local ItemInfos = Quire("ItemInfos")
local SessionData = Quire("SessionData")
local ItemFunctions = Quire("ItemFunctions")
local repr = require(3148021300)

local INVENTORY_KEY = Enums.DataKeys.InventoryKey
local DEFAULT_INVENTORY = {
    Slots = {
        [1] = {
            IsHotSlot = true,
            Item = nil
        },
        [2] = {
            IsHotSlot = true,
            Item = nil
        },
        [3] = {
            Item = nil
        },
        [4] = {
            Item = nil
        },
        [5] = {
            Item = nil
        },
        [6] = {
            Item = nil
        },
        [7] = {
            Item = nil
        },
        [8] = {
            Item = {ID = 1, StackSize = 1}
        },
    }
}

local InventoryService = {InventoryCaches = {}}

function InventoryService:GetInventoryCache(player)
    return self.Inventories[player]
end

function InventoryService:SaveCache(inventoryCache)
    DataStore2(INVENTORY_KEY, inventoryCache.Owner):Set(inventoryCache)
end

function InventoryService:AddItem(inventoryCache, ID, amount)
    amount = amount or 1

    local itemInfo = ItemInfos:GetInfoFromID(ID)
    assert(itemInfo, "Invalid Item")

    local inventorySlots = inventoryCache.Slots

    local function transferAmountToItem(item, amountToTransfer)
        ItemFunctions.IncreaseStackSize(item, amountToTransfer)
        amount = amount - amountToTransfer
    end
    
    -- function is here so I can return to stop both loops when amount == 0, without returning out of this method
    (function()
        -- adds amountToAdd to open stacks of same id
        if ItemFunctions.IsStackable(itemInfo) then
            for slotIndex, slotData in ipairs(inventorySlots) do
                local item = slotData.Item
                local itemIsID = item and ItemFunctions.IsID(item, ID)
                local openStackSize = itemIsID and ItemFunctions.GetOpenStackSize(item)
                local amountToTransfer = openStackSize and math.clamp(amount, 0, openStackSize)

                if amountToTransfer then
                    transferAmountToItem(item, amountToTransfer)
                    -- TODO: Replicate item change
                    if amount == 0 then return end
                end
            end
        end

        -- adds items to empty slots
        for slotIndex, slotData in ipairs(inventorySlots) do
            if not slotData.Item then
                local item = ItemFunctions.SetInfoMetatable({ID = ID, StackSize = 0})
                transferAmountToItem(item, math.clamp(amount, 0, item.MaxStackSize))
                -- TODO: Replicate item creation
                slotData.Item = item
                if amount == 0 then return end
            end
        end
    end)()

    self:SaveCache(inventoryCache)
    print(repr(inventoryCache, {pretty = true}))

    return amount
end

-- initial loading of inventory
Players.PlayerAdded:Connect(function(player)
    local inventoryMetatable = {Owner = player}
    inventoryMetatable.__index = inventoryMetatable

    local inventoryCache = DataStore2(INVENTORY_KEY, player):GetTable(DEFAULT_INVENTORY)
    
    setmetatable(inventoryCache, inventoryMetatable)
    ItemFunctions.SetAllInfoMetatable(inventoryCache.Slots)
    InventoryService.InventoryCaches[player] = inventoryCache

   InventoryService:AddItem(inventoryCache, 1, 3)
end)

Players.PlayerRemoving:Connect(function(player)
    InventoryService.InventoryCaches[player] = nil
end)

return InventoryService
ItemFunctions Module 
local Quire = _G.Quire
local ItemInfos = Quire("ItemInfos")
local SharedItemFunctions = Quire("SharedItemFunctions")

local ItemFunctions = setmetatable({}, SharedItemFunctions)

function ItemFunctions.IncreaseStackSize(item, amount)
    item.StackSize = item.StackSize + amount
end

function ItemFunctions.GetOpenStackSize(item)
    local openStackSize = item.MaxStackSize - item.StackSize
    return openStackSize > 0 and openStackSize
end

return ItemFunctions
SharedItemFunctions Module 
local Quire = _G.Quire
local ItemInfos = Quire("ItemInfos")

local SharedItemFunctions = {}
SharedItemFunctions.__index = SharedItemFunctions

function SharedItemFunctions.SetInfoMetatable(item)
    return setmetatable(item, ItemInfos:GetInfoFromID(item.ID))
end

function SharedItemFunctions.SetAllInfoMetatable(inventorySlots)
    for _, slotData in ipairs(inventorySlots) do
        local item = slotData.Item
        if item then
            setmetatable(item, ItemInfos:GetInfoFromID(item.ID))
        end
    end
end

function SharedItemFunctions.IsStackable(item)
    return item.MaxStackSize > 1
end

function SharedItemFunctions.IsID(item, ID)
    return item.ID == ID
end

return SharedItemFunctions
4 Likes
#2

I’m thinking of making a seperate cache from datastore2’s cache system, this way I can just pass an inventory parameter instead of DataStore2(x, x):Update()

function InventoryService:AddItem(player, inventory, ID, amount)
end)

InventoryService:AddItem(player, inventoryCaches[player], ID, amount)

will have a cache updated function somewhere that updates datastore2’s cache whenever the inventoryCache is updated

feels slightly iffy to be double caching, but datastore2 copies the cache when you request it, so using metadata with ds2 cache isn’t an option and I’d really like to be able to use metatables for item information

#3

Updated the code drastically and I went using another cache system, and more use of metatables, I’m liking this method a lot more but still would love some review if anyone has the time

1 Like
#4

Thanks for sharing. We need more examples on how people are using Datastore2 for us noobs. :slight_smile:

1 Like
#5

I like the cleanliness of your code structure. However, I recommend against making service for sub categories of your data handling system. This can get redundant very fast if your saving more abstract data.

1 Like
#6

Here’s my cursory suggestion: Swap your InventoryService into a providing service that provides and object for use as an inventory.

local inventory = InventoryProviderService:GetInventory(player)
inventory:SetItemAtSlot(1, itemData)

You can OOP the slot if you want. I think in this case the lifetime of the inventory is managed by the InventoryProviderService although I’d maybe move this to the lifetime of a bound player object instance if you have one.

I’d make your utility functions you have be immutable and return new objects too, should help you manage state better imo.

1 Like