I am creating a UserInterface for my game. The following snippet looks through a Frame for TextButtons, resizes them to be slightly wider than the length of the text, and then applies some listeners that bind a keyboard button to each button.
My goal was to create a system that would bring each button into context as they become relevant (and thus visible to the user) however after writing this I felt grossed out by what I’d made.
Any advice on how to redo this or fix this to be more professional and organized would be appreciated.
local ContextActionService = game:GetService("ContextActionService");
local TextHandler = require(script.TextHandler); -- A module that simply allows me to find the width of a given string of text (uses TextService).
local parentView = game.Players.LocalPlayer.PlayerGui.Gui.GameView;
-- Format and hook all the buttons
for _,v in pairs(script.Parent:GetChildren()) do
if v.ClassName == "TextButton" then
-- Calculate the size
v.Size = UDim2.new(0, TextHandler.CalculateWidth(v).x + 20, 1, 0);
-- Init the context
v.Changed:Connect(function(p)
if p == "Visible" then
local hintContextData = require(v:WaitForChild("action"));
if v.Visible then
-- Enable the context
ContextActionService:BindAction(hintContextData.name, hintContextData.fire, false, hintContextData.inputs);
else
-- Disable the context
ContextActionService:UnbindAction(hintContextData.name)
end
end
end)
parentView.Changed:Connect(function(p)
if p == "Enabled" then
local hintContextData = require(v:WaitForChild("action"));
if not parentView.Enabled then
-- Disable the context
v.Visible = false;
end
end
end)
v.Visible = true;
end
end
Hey! I think is good enough but you can always improve, right? Anyway, I think you can leave it like that because it-self the program / system works. A good structure and a cleaner code is always appreciated by someone that’s hiring you, it gives you a better performance (for you as the scripter) and better manipulation of the code without any problem.
An example of this can be:
--[[ Functions ]]--
function printWhatIWant( message )
--// clean code and without any problem
print( message )
end
And is a big difference when you have something like this:
function printWhatIWant(message)
print(message)
end
Is true that both are gonna work, but I think that is better to be the first one compared with the second one, right? Anyway, this is all I have to say.
Edit: Also, yeah there’s a BIG difference on a clean code and a better code, so just have in mind. I’m just giving you some tips to make your code cleaner.
Anyway, I hope this helps you, have a nice day! @Lil_SharkyBoy
instead of using the Instance.Changed event for just 1 property, you should use the Instance:GetPropertyChangedSignal(), which only fires when the given property changes, so instead of
parentView.Changed:Connect(function(p)
if p == "Visible" then
...
end
end)
Hello! Overall, your code looks quite good! What matters is consistency and, at the core, this is exactly that. At no moment was I truly lost.
Now, I’m super happy to see others focusing on maintainability and organization in their code! It’s a skill many don’t practice unless they’re forced to, when it should be the core of any software engineer’s toolkit.
I’d say the main changes I’d introduce is more precise documentation, within reason; more verbose variable names, within reason; and making your code more modular such that the readability of it all is optimized as opposed to the time it takes to write.
A general disclaimer I like to tack onto these things: my code style is far from prescriptive. It is extremely subjective and that’s okay. This is but a reference to help you inform your own style. I also couldn’t test my code because, well, that’s how it is sometimes despite how much that pains me.
local ContextActionService = game:GetService("ContextActionService");
local Players = game:GetService("Players");
local TextHandler = require(script.TextHandler);
local gameView = Players.LocalPlayer:WaitForChild("PlayerGui").Gui.GameView;
local gameViewEnabledChangedSignal = gameView:GetPropertyChangedSignal("Enabled");
local TEXT_BUTTON_X_SIZE_OFFSET = 20;
--[[
Initializes a TextButton's size, visibility, and connects various event
listeners to the central game view configuration as well as
ContextActionService.
@param {TextButton} textButton - The text button to initialize.
@returns {void}
]]
local function initializeTextButton(textButton)
--[[
An event listener to be fired when `TextButton.Visible` changes. Toggles
the actions associated with `ContextActionService`.
@returns {void}
]]
-- TODO: Could hint context data be imported once and stored as a closure'd
-- variable in `initializeTextButton` or is its content dynamic? Would
-- `CollectionService` be better than a ModuleScript to apply actions or is
-- every `functionToBind` unique?
local function onTextButtonVisibilityChangedListener()
local hintContextDataModuleScript = textButton:WaitForChild("Action");
local hintContextData = require(hintContextDataModuleScript);
if textButton.Visible then
ContextActionService:BindAction(
--[[ actionName ]] hintContextData.name,
--[[ functionToBind ]] hintContextData.fire,
--[[ createTouchButton ]] false,
--[[ inputTypes ]] hintContextData.inputs
);
else
ContextActionService:UnbindAction(hintContextData.name);
end
end
--[[
An event listener to be fired when `gameView.Enabled` changes. Updates
`textButton`'s visibility to match.
@returns {void}
]]
-- TODO: Is this event listener even necessary? I thought this was the
-- default behaviour of UI elements? Maybe we can disable a common parent?
local function onGameViewEnabledChangedListener()
textButton.Visible = gameView.Enabled;
end
-- Don't forget to initialize the text button's state.
local computedWidthInPixels = TextHandler.CalculateWidth(textButton);
local paddedWidthInPixels = computedWidthInPixels + TEXT_BUTTON_X_SIZE_OFFSET;
textButton.Size = UDim2.new(0, paddedWidthInPixels, 1, 0);
textButton.Visible = true;
-- Connect our event listeners to our signals.
local textButtonVisibilityChangedSignal = textButton:GetPropertyChangedSignal("Visible");
textButtonVisibilityChangedSignal:Connect(onTextButtonVisibilityChangedListener);
gameViewEnabledChangedSignal:Connect(onGameViewEnabledChangedListener);
end
--[[
Returns all the children instances of `instance` that are of the given
`type`.
@param {Instance} instance - The instance whose children are to be filtered.
@param {string} type - The type of children to retrieve.
@returns {Instance[]}
]]
local function getChildrenOfType(instance, type)
local children = instance:GetChildren();
local childrenOfType = {};
for _, child in pairs(children) do
if child:IsA(type) then
table.insert(childrenOfType, child);
end
end
return childrenOfType;
end
-- Initialize all relevant text buttons. Perhaps we should implement some error
-- diagnostics if any initialization fails?
local textButtonsToInitailize = getChildrenOfType(script.Parent, "TextButton");
for _, textButton in ipairs(textButtonsToInitailize) do
initializeTextButton(textButton);
end
If you or anyone else has any questions, feel free to ask! (That is the purpose of these things after all).
Outside of Roblox I work both on proprietary applications with teams ranging between 5-10 people and open-source software. Learning in these environments forcibly kept me in check, always having to write code that someone might be reviewing who isn’t too well-versed in either the component I was developing or the entire project.
Furthermore, I RM for a non-profit organization (whose name I won’t plug because I don’t know the policies on that; anyone can ask in DM?) that teaches others to program.
It’s a shame, really, that Lua doesn’t have a really reliable linter (seeing that luacheck is so-so, especially when compared to other developer ergonomics across languages). It means a lot of packages and styles I’ve seen accidentally create their own pitfalls or just aren’t really thought out.
(Didn’t see that in your code though! All’s good.)
I’ve always wanted to get into open source but I’ve always been to nervous. Any recommendations to ease myself in? Its proabably just a typical case of imposter syndrome. I seem to find myself constantly falling into the trap of ugly code and I want my work to look and feel better.
It hadn’t even occurred to me that one could define functions within functions until I looked at this snippet. I think that this little bit of knowledge alone will greatly improve the sanity of my code base.
If you don’t have a GitHub account already, definitely make one. Most open-source repositories have a contributing.md or CONTRIBUTING.md file that explains how you can help their project. If not, you might be surprised how willing some projects go to make sure you’re feeling included.
It certainly doesn’t hurt to start a project yourself.