So this is the code that I used for making an inventory with “add” and remove “items” function,
I just wanted to know if I’m not doing anything that is not necessary and if my logic is good for this situation, I want any kind of feedback and comments so I can upgrade this to its better form.
The code is long so if you have enough time to read it it is here. New one here. Thanks for reading!
I’m about to be really mean to your code. Sorry.
I can’t tell you if your logic is good because your code is totally unreadable. In general, your variable names are bad. It needs to be clear what each variable represents mostly just from the name and a bit of context. Don’t use abbreviations too much, saving on letters isn’t worth the readability loss. Don’t call variables “table” or “bool”. Explain what idea they represent, not what type they are. Have spaces between operators and operands. “1+1” is not good, “1 + 1” is good. Especially also true for commas. table.insert(inventory,i,copy) is bad, should be table.insert(inventory, i, copy). Don’t have the 3rd argument to numeric for loops if it’s 1. Leave it out. Don’t specify the position to insert when calling table.insert if you want to insert onto the end of the table. Just input the table and the value to insert, and it will put it on the end by default.
In the AddItem and RemoveItem methods, there’s 9(!!!) levels of indentation, with deeply nested if statements and for loops, seemingly with all sorts of weird special cases. It’s impossible to follow your logic, not least because of the cryptic variable names and complete lack of comments. I don’t know what your system does, but I know it doesn’t need to be written like this. Split repeated code into separate (well named) functions and call those instead, and if you need to handle special cases then try to do it by returning, breaking or continuing early instead of having nested ifs.
I tried mentioning all the things that are wrong but it’s just too much. Instead I tried fixing it. Some parts of the code can be changed in isolation:
local i = #inventory+1
local copy = cloneTable(itemD)
table.insert(inventory,i,copy)
inventory[i].Amount = 1
becomes
local copy = cloneTable(itemD)
table.insert(inventory, copy)
copy.Amount = 1
local i = #inventory+1
local copy = cloneTable(itemD)
table.insert(inventory,i,copy)
if amount >= itemD.MaxAmount then
inventory[i].Amount = itemD.MaxAmount
elseif amount < itemD.MaxAmount then
inventory[i].Amount = amount
end
amount-=inventory[i].Amount
becomes
local copy = cloneTable(itemD)
table.insert(inventory, copy)
if amount >= itemD.MaxAmount then
copy.Amount = itemD.MaxAmount
else
copy.Amount = amount
end
amount -= copy.Amount
local function funcA(inventory, itemD, numToAdd)
local times = math.ceil(numToAdd / itemD.MaxStackSize)
for x = 1, times do
local copy = cloneTable(itemD)
table.insert(inventory, copy)
if numToAdd >= itemD.MaxStackSize then
copy.Amount = itemD.MaxStackSize
else
copy.Amount = numToAdd
end
numToAdd -= copy.Amount
end
return numToAdd
end
local function funcB(inventory, itemD, numToAdd)
local copy = cloneTable(itemD)
table.insert(inventory, copy)
copy.Amount = numToAdd
end
function system:AddItem(player, itemType, numToAdd)
local inventory = inventories[player].Inventory
local itemTypeData = itemData.Items[itemType]
if not (inventory and itemTypeData) then return end --should probably error() instead?
if not numToAdd then return end --should probably error() instead?
if itemTypeData.Stackable then
--Explain
if getItem(inventory, itemTypeData) then
--Explain
--[Could be split into separate function
local itemStacks = {}
for _, inventoryStack in ipairs(inventory) do
if inventoryStack.Id == itemTypeData.Id then
table.insert(itemStacks, v)
end
end
--]
--Explain
--[Could be split into separate funciton
local smaller, bool = findSmall(itemStacks)
for _, inventoryStack in ipairs(itemStacks) do
if inventoryStack.Amount == smaller then
local sum = numToAdd + inventoryStack.Amount
if sum <= itemTypeData.MaxStackSize then
inventoryStack.Amount += numToAdd
else
local numToAddToRest = itemTypeData.MaxStackSize - inventoryStack.Amount
inventoryStack.Amount = itemTypeData.MaxStackSize
numToAdd -= amountToRest
numToAdd = funcA(inventory, itemTypeData, numToAdd)
end
end
end
--]
else
if numToAdd <= itemTypeData.MaxStackSize then
funcB(inventory, itemTypeData, numToAdd)
else
funcA(inventory, itemTypeData, numToAdd)
end
end
else
for x = 1, numToAdd do
funcB(inventory, itemTypeData, 1)
end
end
end
Try and think of some good function names for funcA and funcB. If you manage to split the two suggestions I made with comments into separate functions, it would actually be really easy to follow the logic of what happens when adding items to an inventory.
function system:AddItem(player, itemType, numToAdd)
local inventory = inventories[player].Inventory
local itemTypeData = itemData.Items[itemType]
if not (inventory and itemTypeData) then return end --should probably error() instead?
if not numToAdd then return end --should probably error() instead?
if itemTypeData.Stackable then
--Explain
if getItem(inventory, itemTypeData) then
local itemStacks = funcC()]
funcD(itemStacks)
else
if numToAdd <= itemTypeData.MaxStackSize then
funcB(inventory, itemTypeData, numToAdd)
else
funcA(inventory, itemTypeData, numToAdd)
end
end
else
for x = 1, numToAdd do
funcB(inventory, itemTypeData, 1)
end
end
end
Such an improvement over what we started with.
I’m gonna fix it so you guys can read it!
I compacted it as much as I can, here, I’m just having trouble with the removeItems function, since sometimes it doesn’t delete all the items with amount of zero, for example in this case I setted to zero the amount of three items, but it just deleted two :
Inventory has 5 items.
Deleted 2 items
So I wan’t to know how I can improve it , If you can help me with that point and the others again I will be very happy!
I think that @ThanksRoBama has been unduly harsh here, and your response shows the issue with their kind of feedback. You don’t want to compact your code as much as you can because this makes it less readable. Here’s my feedback on your script:
Whilst the points RoBama makes about formatting are correct (don’t abbreviate your variable names, don’t name variables after their datatype, add spacing between arguments etc.) these are not as severe as they’re is making them out to be.
I found that, in general, I could understand the logic of your code, and follow your train of thought.
The one point I will agree with the severity of however is commenting. Any line that isn’t entirely self-explanatory should be explained with comments. In a script this short it isn’t a huge issue, but if you end up with tens of thousands of lines of code you’re gonna lose track of it, and you’re going to regret not commenting.
Concerning the levels of indentation in your program, I think this is due to an ignorance of Curly’s Law: Do One Thing. Also known as the Single-Responsibility Principle (or SRP for short), this is the idea that any part of your code should do exactly one thing.
For example your system:AddItem() method could be split into :AddStackableItem() and :AddUnstackableItem(), and your :AddItem() method would simply determine if the item was stackable or not and call the correct method, for example
if itemD.Stackable == true then
self:AddStackableItem(player, item, amount)
else
self:AddUnstackableItem(player, item, amount)
end
Note that I have assumed that plr is short for player and itemD is short for item data.
To lower the indentations further you could reformat the way you do your assertions. This is best explained through example:
if inventories[player] and itemD.Items[item] then
local inventory = inventories[player]
local itemD = itemData.Items[item]
if amount then
...
end
end
would become
local inventory = inventories[player]
local itemD = itemData[item]
if not inventory then
warn("Player inventory does not exist")
elseif not itemD then
warn("Item does not exist")
elseif not amount then
warn("Amount argument is null")
else
...
end
This makes the code less stacked, and separates the assertions from the logic, making the code more readable. Additionally, if one assertion fails then the rest are not needlessly checked which helps efficiency.
However, it should be noted that since these assertions are necessary and cannot be removed the stacking doesn’t affect code performance, only readability, although the importance of readability cannot be overstated!
The use of warn() and error() are important in debugging, but you’ll want to be careful with these. If warn() is called then an orange output is printed to the console - useful for telling you the developer that a problem has been averted, but you should have a look at it.
error() is different in that it stops the script - this should be used for “oh no, something has seriously gone wrong and if this thread continues then something really bad could happen”. You don’t want this in any game-critical scripts, such as an inventory system.
Speaking to the logic of your script I’m happy to say that it is perfectly fine, there are no obvious changes I would make.
Finally I should recommend that you do not take this feedback as a set of corrections to apply to your script, but as advice to apply to all of your work going forwards. You should be actively looking to write code that this feedback would not apply to.
Best of luck with your game 