findPlayer should search by 1 type at a time (bug/exploit fix)

[lionbryce]

Someone noticed that setting your steam name to someone else's in game name (or a portion of it) can goof up targeting due to the order that it scans

example:

player 1's nickname is "bob", steamname is "ABC123 OtherWords"
player 2's nickname is "123xyz", steamname is "The Crush Inator (idk)"
doing "/pm 123 AHHHHHH!" would target player1 as their steamname has 123 in it

I threw on some extra conditionals to avoid ever doing 4 loops of player.GetAll(), worst case is gonna be 3

could throw in a length check as 33 characters is too long for any of them and steam64s have a fixed length (or at least a minimum length)

[FPtje]

Thanks for the PR. Since this function has quite precise behavior, it'll need a close review. I'll take a look when I have time.

[FPtje]

This PR gives a very interesting insight. I was under the impression that findPlayer would always first match by UserID, then by SteamID, then by nickname, and then by steam name. This is not the case, though, because it checks everything per player. The order in which players are iterated over can decide who gets chosen! That's not desirable behavior, because that's hard to predict.

If I were to write a unit test for this function, it would be that it should always return the same player, regardless of the order of players.

I think your PR is great and really solves a problem with findPlayer Let me clean it up and merge it!

[FPtje]

I think this won't even be (much) slower than the old code. The old code did one loop, with 4 things inside the loop. This thing does 3-4 loops at most, and does one thing per loop.

I think in some cases (e.g. when matching a UserID), this version might actually be faster, because it shortcuts the other loops.

Doing a for-loop like this is cheap, even in the worst case I think this function will have very good performance.

Thanks for this PR!

[FPtje]

I don't think this loop should be combined with the above loop, because then the order of players can influence the answer again

[FPtje]

Good idea to add this check, it saves a loop

[FPtje]

A future optimization here could be to only run this loop when info is a valid SteamID64. SteamID64s have a specific format to them, and not nearly every int is a valid SteamID64.

A function isValidSteamID64(some_number) is a bit of a challenge to write, though, as it requires a thorough understanding of what makes a valid SteamID64.