Inventory/Container System Review

Help and Feedback Code Review
#1

Hey everyone,

Today I just wrote my second take on an inventory system (in this case I called it a container system). Since this is only my second time I wanted to get people’s opinions on what could be improved. This is the server side OOP object that will be saved to the my DataStore. The data property of the Container object might look something like this for an example:

Container.Data = {
     {Id = "Sword", Amount = 5, Position = 10};
     {Id = "Wood", Amount = 65, Position = 9};
}

local Container = {}
Container.__index = Container

--// Functions \\--
function Container.new(owner, name, size, data)
	local self = setmetatable({
		Owner = owner;
		Name = name;
		Size = size or 0;
		Data = data or {};
	}, Container)
	return self
end

function Container:IncrementSize(number)
	self.Size = self.Size + number
end

function Container:GetOpenPosition()
	for pos = 1, self.Size do
		local PositionTaken = false
		
		for _,item in pairs(self.Data) do
			if (item.Position == pos) then
				PositionTaken = true
				break
			end
		end
		
		if (PositionTaken == false) then return pos end
	end
end

function Container:GetItemIndex(pos)
	for index,item in pairs(self.Data) do
		if (item.Position == pos) then
			return index
		end
	end
end

function Container:Add(id, amount, pos)
	local OpenPosition = pos or self:GetOpenPosition()
	local ItemData = self.Shared.ItemData:GetData(id)
	
	if (not OpenPosition or not ItemData) then return end
	
	if (ItemData.CanStack == false) then
		table.insert(self.Data, {Id = id, Amount = 1, Position = OpenPosition})
	elseif (ItemData.CanStack) then
		if (amount > ItemData.MaxStackSize) then
			table.insert(self.Data, {Id = id, Amount = ItemData.MaxStackSize, Position = OpenPosition})
			
			self:Add(id, amount - ItemData.MaxStackSize)
		else
			table.insert(self.Data, {Id = id, Amount = amount, Position = OpenPosition})
		end
	end
end

function Container:Remove(pos)
	self.Data[self:GetItemIndex(pos)] = nil
end

function Container:IncrementAmount(pos, num)
	local Item = self.Data[self:GetItemIndex(pos)]
	local ItemData = self.Shared.ItemData:GetData(Item.Id)
	local NewAmount = Item.Amount + num
	
	if (ItemData.CanStack == false) then return end
	
	Item.Amount = NewAmount
	
	if (Item.Amount < 1) then
		self:Remove(pos)
	elseif (Item.Amount > ItemData.MaxStackSize) then
		Item.Amount = ItemData.MaxStackSize
		self:Add(Item.Id, NewAmount - Item.Amount)
	end
end

function Container:Move(pos1, pos2)
	local Pos1Item = self.Data[self:GetItemIndex(pos1)]
	local Pos2Item = self.Data[self:GetItemIndex(pos2)]
	
	if (Pos2Item ~= nil) then
		local Pos1Data = Pos1Item
		local Pos2Data = Pos2Item
		self:Remove(pos1)
		self:Remove(pos2)
		self:Add(Pos2Data.Id, Pos2Data.Amount, Pos1Data.Position)
		self:Add(Pos1Data.Id, Pos1Data.Amount, Pos2Data.Position)
	else
		self:Add(Pos1Item.Id, Pos1Item.Amount, pos2)
		self:Remove(pos1)
	end
end

function Container:Stack(pos1, pos2)
	local Pos1Item = self.Data[self:GetItemIndex(pos1)]
	local Pos2Item = self.Data[self:GetItemIndex(pos2)]
	local ItemData = self.Shared.ItemData:GetData(Pos1Item.Id)
	
	if (Pos1Item and Pos2Item and Pos1Item.Id == Pos2Item.Id and ItemData.CanStack) then
		local NewAmount = Pos1Item.Amount + Pos2Item.Amount
		
		if (NewAmount > ItemData.MaxStackSize) then
			Pos2Item.Amount = ItemData.MaxStackSize
			Pos1Item.Amount = NewAmount - ItemData.MaxStackSize
		else
			Pos2Item.Amount = NewAmount
			self:Remove(pos1)
		end
	end
end

return Container
2 Likes
#3

This syntax is a bit strange. It may just be me, but I find that it does not clearly show the order of the operations. You also switch to using equality operators (== false). Why not consistently use equality operators (== nil)?

#4

Should I switch it to:

if (OpenPosition == nil or ItemData == nil) then return end

?

And for the second point you made I don’t understand what you are referring to… Can you quote a piece of code that you are trying to point out?

#5

Yes, that is better.

#6

I see the benefit of using nil in this case but if I do set CanStack to false will the if statement still work?

For example:

local A = false

if (A == nil) then
   print('Will this print?')
end

I mean, I’ll just test it I guess…

#7

It doesn’t print so that is not possible.

#8

Shouldn’t need linear search to find item index, you should change the schema to use the dictionary. I recommend item name as string, and item data as the value.

#9

This would not work as I may have two of the same item. I am not creating a list but a grid system of sorts.

#10

I would point out the purely using “if not x then” handles more cases and it looks cleaner.
Although I do agree that it doesn’t need to be inside brackets.

1 Like
#11

Glad to see Western Horizon is making progress!

#12

Oh, yeah. Most the stuff I am doing is behind the scenes because we don’t want to release little tiny QoL updates. We are going to release one big update. It obviously won’t have all the features everyone wants but it will be close.

#13

why not just use.

if not ItemData.CanStack then
1 Like
#14

Consistency. It is fairly subjective so either way is correct, you could use not, but in that case, you may as well use not for all cases where you need to compare a boolean.