Trading system review

Help and Feedback Code Review
#1

(Sorry for bad english)
So I made a working trading system but I just realized that players can leave the game while the server swap players items and I was wondering if this could be an issue? if so, how can I handle/improve it?

Here’s an example of my swapping script

local function SwapItems(plr1, plr2)
  local plr1Inventory = Datas[plr1.UserId].Inventory
  local plr1Offers = Datas[plr1.UserId].Offers

  local plr2Inventory = Datas[plr2.UserId].Inventory
  local plr2Offers = Datas[plr2.UserId].Offers

  for i, v in pairs(plr1Inventory) do
    for i2, v2 in pairs(plr1Offers) do
       if v == v2 then
          table.insert(plr2Inventory, v)
          table.remove(plr1Offers, v)
          table.remove(plrInventory, i)
      end
    end
  end

  for i, v in pairs(plr2Inventory) do
    for i2, v2 in pairs(plr2Offers) do
       if v == v2 then
          table.insert(plr1Inventory, v)
          table.remove(plr2Offers, v)
          table.remove(plr2Inventory, i)
      end
    end
  end```
6 Likes
#3

Pass a check at the end of the function scope to verify both players are still ingame, if one or the other isn’t assume they left during the swapping process and restore the respective items to each player’s inventory.

3 Likes
#5

As Syharaa said, you could use a verifier to see if the player is still in game in which case you could use game.Players:FindFirstChild(game.Players.LocalPlayer) and maybe add like a table that get’s all the items that we’re traded and maybe have them in a list.

#7

When a table is being iterated in a for loop, or any logic loop for that matter i.e ( while true do, repeat until ) the thread yields while the table is being iterated or while the loop is being ran, the code will not run asynchronously and it will not happen almost instantly because depending on the number of items being iterated more time is being taken for the loop to complete, therefore yielding the thread even if by a quarter of a second.

Remember he is iterating through both player’s inventory’s and their offers one after the other, this can cause a yield and during this time a player can leave and mess everything up, hence we pass a check AFTER not BEFORE and in any case that’s just common sense always pass a validation check after a process has been done not before.

#8

You cannot check if a Player’s parent is nil because the parent will always be the Players object, I personally would have the swapping system setup in states, that is the swapping state, validation state, finished state, and connect a player leaving event to both players in the process and then disconnect those events after the process is done. When the player leaved event has been fired we know the player left, therefore roll back changes. ← @Xbscure

#10

The player object gets destroyed as far as I know and I don’t know if destroyed objects have their parents set to nil, because iirc if a player leaves and you try to index the player object to check their parent it may throw an error stating that player object itself is nil.

With loops the code does not run asynchronously unless you run the loop in a separate thread within that thread i.e coroutines. Remember when an table or array is being iterated, there is a delay even if it’s a micro second, it’s still a delay and that time taken to iterate the table increases with the amount of iterations that have to take place, think of it as saying your abc’s as fast as possible so that you could go outside and play, you’re still taking time to do that right? so what if you had to say your abc’s and count to 50 twice, this would increase the time taken for you to go outside and play.

#11

And you’re right about that but it’s better to not taking chances, because OP may actually implement a wait() function in his code for whatever reason be it to give something time to load or whatever the case may be that’s why I initially said to implement the code after everything has been done.

#13

Yes you’re right their parent is set to nil, my bad but I don’t see the need to check if the object is nil and then check if it’s parent is nil, why not just check if the player itself is nil or not?

and also like I said, better safe than sorry this isn’t my code or yours as far as I can see so I’m not sure what logic the OP really has in mind but I don’t see how it’s sensible to validate then run the process that’s pretty backwards, even if it’s instantaneous it should be done after the processes have occurred plain and simple.

I also don’t see why you think a player can’t leave during the process, it is possible, simply close your client lol, if what you’re trying to say is why would someone leave during the process? well my friend, some people have bad internet (self explanatory)

last point I’m not sure if we’re allowed to go on like this lol so I guess I’ll just stop here and let the OP decide what approach he wants to take, also stay safe during this time bro <3

1 Like
#14
#15