So sometimes when I’m bored I try to make things from other games and today I tried making a random position thing, from piggy type games.
The code is very simple and works, but can it be improved?
Here is the code:
local PossiblePositions = {
game.Workspace.R.Position + Vector3.new(0,6,0), -- Idk why I decided to call it rgb
game.Workspace.G.Position + Vector3.new(0,6,0),
game.Workspace.B.Position + Vector3.new(0,6,0)
}
game.Workspace.Button.ClickDetector.MouseClick:Connect(function()-- This was just for a test
local RandomPosition = math.random(1,3)
game.Workspace.MainPart.Position = PossiblePositions[RandomPosition]
end)
Hey there! I’d recommend writing code that is less hard-coded and more extensible. A possible improvement is as follows:
local POSITION_OFFSET = Vector3.new(0, 6, 0);
local POSSIBLE_POSITIONS = {
workspace.R.Position + POSITION_OFFSET,
workspace.G.Position + POSITION_OFFSET,
workspace.B.Positon + POSITION_OFFSET,
};
local clickDetector = workspace.Button.ClickDetector;
local mainPart = workspace.MainPart;
--[[
The `Random` object to be used in this script, pulling a seed from Roblox's
internal entropy source. At the moment, this is more reliable than using
`math.random` after being primed using `math.randomseed`, removing unneeded
implict state.
]]
local random = Random.new();
--[[
Returns `true` if the given `value` is a valid array.
@param {unknown} value
@returns {boolean}
]]
-- TODO: Decouple function to a type-checking or value assertion module.
local function isArray(value)
if type(value) ~= "table" then
return false;
end
local propertyCount = 0;
for _ in pairs(value) do
propertyCount += 1;
end
return propertyCount == #value;
end
--[[
Returns a random element within the given array.
@template T
@param {T[]} array
@returns {T}
]]
-- TODO: Decouple function to an array utility module.
local function getRandomElement(array)
assert(isArray(value));
local randomIndex = random:NextInteger(1, #array);
return array[randomIndex];
end
--[[
The event listener to be triggered every time `clickDetector` is clicked.
@returns {nil}
]]
local function onClick()
local randomPosition = getRandomElement(POSSIBLE_POSITIONS);
mainPart.Position = randomPosition;
end
clickDetector.MouseClick:Connect(onClick);
I think this is way too overkill. The OP’s code works just fine as is.
The only thing in the OP I would change is maybe making it shorter by localizing Workspace and the constant vector. Apart from that, maybe use math.random(#PossiblePositions) to make it easily extensible.
Yeah, it’s very clear that this is an explosion in complexity for a typical prototype. My background typically puts a heavy emphasis on maintainable and extensible solutions.
Furthermore, adhering to these core principles naturally progress to more robust and maintainable solutions that can be tested easily.
TL;DR: For the OP’s use-case of a quick-and-dirty use, non-extensible patterns in a prototype is fine, however production code should lean towards the side of maintainability; never resort to copy-and-paste programming.
I would suggest when choosing the random number instead of having math.random(1, 3) use math.random(1, #PossiblePositions). This ensures that when updating the amount of objects inside the table the random value will stay up to date else you will have to change it to math.random(1,4) etc and that will be boring and not efficient
If someone handed me that code in a job interview, I would politely ask them to leave.
There’s a learnable lesson here about making your code too dynamic or “enterprise quality”.
What you’ve written is not good. You can probably recognize that it’s needlessly verbose, but beyond that, it’s just plain inefficient.
Every time someone clicks, you’re checking to see if the POSSIBLE_POSITIONS table is an array, and calling a function to get a random index for it. This is fine, except that you’re assuming that
a table that’s defined as a ‘constant’ will change, so you’re checking to make sure it’s still an array
the check to make sure the table is an array doesn’t actually do that
you’re potentially throwing without handling the error – events sink errors anyway, so all you’ve done is make a click-to-error button
You’ve also gone ahead and provided code that’s heavily annotated with stuff that has no bearing to Roblox without explaining any of it, which is less than unhelpful to the OP.
As the actual post (sorry for the sidetrack @SpacialEthanRB!), I’d say that the best advice I have is not hard code the values, like @TheEdgyDev said. At the moment if you decide you want to change what the offset for each position is down the line, you’ll have to change each line manually. You can see why it would probably be better to just have a variable with that offset stored in it so that you only have to change one line.
You should also probably use Random over math.random. It seems more verbose but it’s generally more random, which is what you want in this situation.
Beyond that, this isn’t how I’d personally go about this. I’d probably use CollectionService tags rather than manually filling a table with the parts you’re teleporting stuff relative to. Let me know if you want more information on how to do that.
Looping through the whole array just to check if its not a dictionary is not very efficient if you have multiple positions. All you have to do is this
local Array = false
for i,v in pairs(value) do
if i ~= 1 and #value ~= 0 then -- extra check to prevent an edge case where the first index is 1 but the next index might be a non numerical increment index for example dude = {"hello", Cool = 1}
break
else
Array = true
break
end
end
return Array
Again this is only if you have a LOT of positions, and is kinda micro optimization ngl
Also why isn’t indenting working for me Nevermind now it is
As an aside, while the case could be made against the logistics of differentiating an array-like table amongst others, the proposed efficient solution fails due to the following factors:
The entries returned by pairs is inconsistent, therefore any valid integer index that fails the check
Such as 2 before 1 is computed will be a false negative.
Furthermore, since Lua does not support sparse arrays, referencing against numeric indexes doesn’t guarantee it’s a valid comparison.
While it’s possible that there’s a more efficient way of seeing if a value is an array, the implementation I used is the most robust one I’ve seen thorought most Lua libraries. However, the solution you’ve provided doesn’t guarantee the value is indeed an array.
Thanks for the criticism, genuinely. My familiarity with Roblox error handling is so-so; knowing that assertions are ate up was a good refresher. Also, it does make no sense to assert that an immutable array is, well, ever mutated.
@SpacialEthanRB I definitely second the use of CollectionService like @Dekkonot suggested. Tags are much more manageable than hardcoded values strewn about various modules.
Now create that same table with a few properties. 1 is called first on this now non-array. It fails the first condition, setting array to true and thus has thrown a false positive. It computed a non-array was an array. Ergo, said solution doesn’t function as intended.
No problem, man! I know an alternative is to count the number of properties then insure each numerical index exists, although the # operator does the same computation and pairs in the Luau VM is so optimized I consider its overhead relatively negligible.
A sparse array is just an array where the elements do not have consecutive integer indexes.
An example of sparse arrays in another language, such as JavaScript, would be [1,,3] where the length of the array is computed as three although only 2 elements exist. Lua doesn’t support this, although sparse arrays can be found at runtime for various reasons.
local arrayOne = {1, nil, 3};
print(#arrayOne)
-- Prints "3"
local arrayTwo = {
[1] = 1,
[3] = 3,
}
print(#arrayTwo)
-- <!> Prints "1" <!>
While this example is impractical, it’s not unrealistic to believe some object constructor or the like creates an array-like dictionary with holes, therefore checks such as # or ipairs become void. I believe this is why most implementations fall back on counting properties in a table and seeing if it’s equal to the table’s length assuming it has no holes.
local clickDetector = workspace.Button.ClickDetector
local mainPart = workspace.MainPart
local PossiblePositions = {
workspace.R.Position + Vector3.new(0, 6, 0), -- Idk why I decided to call it rgb
workspace.G.Position + Vector3.new(0, 6, 0),
workspace.B.Position + Vector3.new(0, 6, 0),
}
clickDetector.MouseClick:Connect(function()-- This was just for a test
local RandomPosition = math.random(1, #PossiblePositions)
workspace.MainPart.Position = PossiblePositions[RandomPosition]
end)
You should use variables that’s all I can say on your code as well as use workspace rather than game.Workspace since it will index game, then workspace. It’s your choice here.
Instead of math.random (1, 3) you should do math.random(1, #PossiblePositions). No matter how many positions you defined, you won’t have to write the random positions in math.random, it will just choose 1 out of the given possible positions.
Nevermind now it is