This is a module script that makes some common tasks, or tedious ones, easier than before.
This is not intended to be used as a learning tool for coding, and nor do I claim to be an expert in coding. So.. take the quality and readability of this module with a little grain of salt.
For some reason, some of this text isn’t readable on mobile. Did you use a Unicode font generator to stylise the text? Please use the tools available in the post editor instead to stylise text.
The contribution is welcome but I have several bones to pick with this resource and I haven’t even looked through the source code yet.
The first is to suggest yielding at the start of every script needing to use these. Commands should be available on demand upon being required. That is to say, whether requiring by id or inserting the module, the functions should be returned to the requiring script. Use of _G is not necessary and should only serve as a fallback.
On the topic of _G, I dislike how your ModuleScript hogs variable names in the global table, which can pose conflictions with other resources and developer uses of _G. Yes they can be changed but I don’t think that should be the default behaviour. It’d be much more preferable to reserve a slot in _G with a table, which includes your functions as members.
Finally, the API for this module is plain confusing and unintuitive, as are the names for some of these functions. For example, the ChangeSame function accepts a data value first, then varargs next for a collection of instances to change then tops that off with a table of what properties to change. It’s a rollercoaster and doesn’t make much sense for use. I can already see awkward conventions like this hurting readability when used.
It’s a good starting point but I think you should revise the resource some and look at other open source utilities to learn how to better tailor your own for the ultimate developer experience. As it is now, I’d say it fetches for a learning resource at best but doesn’t have any standing in production-ready games.
I did use some sorta Unicode font generator since the current stylizing tools here are very limited (to my knowledge), although I’d fail to see why anyone would be coming to a topic like this on a mobile device. Perhaps I should still fix it eventually though.
I understand the dislike to make something have to yield in order to fetch this way, but the suggestion to yield is only in the case on the example requiring you to use _G. commands and I understand this is not indeed necessary, this was only a method for easy set-up for those who may not understand scripting well enough to know how to set it up properly on their own. If ROBLOX ever allows LocalScripts to fetch ModuleScripts via their ID then that would’ve made things much simpler. So if they dislike that method they just need to move it somewhere where both kinds of scripts can find it without yielding.
As it states,
, not the only methods.
As for the argument you’ve made against the function ChangeSame:
• Yes, it may look a little hard to read when in use. This purpose of these functions is to make things easier to do, not read. IMO it works well enough (at least how I’ve been using it in my own time).
• What exactly is confusing about how it’s set up? You first set the value you want to be set, the instance(s) that you’d like to change, and then all of the properties you wish to change. This is recommended for instances that have multiple properties that have the same value, such as a part with their 6 faces. It helps to save a little space.
I do understand that this perhaps isn’t the best way to convey everything to the players. It just seemed to fit at the time. Do you have any personal recommendations that I should look at? I’d greatly appreciate any you can give.
There are many styling tools you have access to, from headings to blockquotes and HTML blocks. It’d be worth experimenting with formatting at some point in time. No forum comes without a set of tools to stylise with; if it does, it should have them.
Not everyone has access to a computer, whether because they don’t own one or aren’t within the vicinity of one. For example, I browse the DevForum often with free time in school and I read lots of threads on my phone. Anyone can view any topic from any device.
Programming is about both efficiency in the doing and readability in the writing. If neither can be guaranteed, you start falling into holes and leading other newer developers into such holes as well indirectly, especially in an OSS work. Lack of readability in code can impede maintainability, which is a very important aspect to have in your code.
You are capable of ensuring easy to do and easy to read. If you can’t guarantee one or the other, it devalues the resource (subjective) as a production-ready tool and a ground for new developers to work with or from.
This is in reference to the entire module, not just ChangeSame. The API feels unintuitive and confusing to use. If it’s specifically targeted at a new developer audience, then you need to write clear code and provide appropriate documentation as well, along with an API that’s readable and easy to follow. The order of parameters for ChangeSame, as well as its name, are just one example.
I don’t want to go off on a tangent but there are already several other things I hold the same view on. There are a few that I can quickly name without picking apart the whole resource:
_G.new: Unclear as to what new is. Remember that you’re assigning this to the global table. A name that clearly describes what you’re doing would be more appropriate, such as create or NewInstance. This also breaks convention with how the rest of your functions follow PascalCase while this function is in camelCase.
_G.PN:PN is an obscure name and doesn’t quickly speak to the developer what it’s for without reading documentation, exploring the code or remembering what it does from either of the aforementioned two methods. Two-letter variable names are horrid for readability. Libraries should be clear and readable as they incorporate functionality into the script.
_G.Rando: Again on readability. Rando is a weird alternative for Random. Variable names should be clear and appropriate on what they’re going to accomplish. For me, even that one missing letter offsets my workflow.
Now that I actually have had the time to look through the source code, I have additional feedback that I need to give. There are a few conventions that need to be changed and don’t employ best practices. As a casual use library this may not concern many but as a learning resource it should.
Yielding at the start of the script’s run time.
LocalScripts only run when placed inside a container that supports the running of client-side code, those of which can link back to the player in some way. There is absolutely no need to yield at the beginning of the script’s lifetime and delays when the code adds the functions to _G.
Poor and inconsistent indentation.
This one should go without saying.
Everything from the squared box down should be moved back a tab:
So on. Code writing is majorly preferential but it helps to standardise even your tabbing so other developers exploring your code can see it in a clear and common fashion. That’s to say, tab should only be used for chunks entering scopes, not for new lines or the like. Ends should be placed at the same tab width as the scope opening statements.
Frequent jumps between use of next and pairs in for loops.
This one is self explanatory. The library can’t make a decision on whether to use pairs or next in the code, which butchers its readability. Please choose one or the other. In the case that the indice is not necessary, I recommend using ipairs given the speedup used in the new VM and how it’d be the proper iterator to use.
Lack of error handling.
A lot of this code makes wide assumptions, especially in the realm of the multi-instance operations. These are easily error bound due to instances sporting different classes that can be missing the specified property. This is more of an implementation issue on the developer’s half but I believe that a beginner resource should also handle error cases for them.
The code should make effort to validate what it’s doing before doing it. This is especially the case for ChangeSame which is a relative rollercoaster that’ll be addressed later.
_G.new ignores instancing performance issues.
_G.new is essentially what RbxUtility.Create was, but poorly done. One of the already-major issues I noticed is that if a Parent argument is specified, its setting isn’t deferred until all other properties are set. Parent should always be set last. See this thread:
I have a similar library on my GitHub page, titled Instance2. It follows the convention of being able to instance while specifying properties to be set as well, but in accordance with the above thread the setting of Parent is deferred. It also fixes vanilla Instance.new. You can view the source here:
As for final mentions, there’s a weird piece of code that I don’t understand the purpose of. Attempting to work with it throws errors, regardless of how I do it.
if type(i)=='number' then
v.Parent=c
else
c[i]=v
end
Apparently if the type of index i is a number (and not the value), it attempts to parent the value to the created instance c. Is this meant to specify children to be added to the table? If so, not only is this not documented anywhere but this is conventionally awkward to allow a table of properties to also specify children to be parented. A different convention or extension function needs to be available here.
Conventions for _G.ChangeSame are unintuitive.
I’m sure I already outlined this earlier.
The order of parameters are as follows:
Value to be used for each passed property
List of instances to be changed
Properties to be changed
I think that this list is jumbled and it’d make more sense to put them in an comfortable order typical of standard convention when scripting, such as passing the items to change first instead.
List of instances to be changed
Properties to be changed
Value to be set
This function feels overall awkward to use so I don’t have many alternate formatting suggestions to offer at the moment.
This is all I have for now. I think I spent a little too much time here. The point mainly resides in my first post and this is just meant to clarify some items as a response.
That… was a mountain of information to get through.
First, I’d like to correct a couple of misunderstandings that you seem to have caught:
This wasn’t made with the intention of the source code to be read from (I don’t forbid doing so, of course), nor was it intended to be particularly used by newer developers. This was stated at the top of the post:
This also wasn’t intended to necessarily be a learning resource either. As for mentions of tabbing, that’s all entirely bias based on the developer. Your ideas of good indentation in code may not always match up to everyone else’s, and it isn’t as though people who have some knowledge of scripting can’t figure it out. Not that you didn’t point out any genuine flaws, but again, it wasn’t made with the intention to have people sift through the whole thing. If that needs to be clarified in the post then I will do-so when I eventually go back to fix it up.
Now with that out of the way…
I suppose the names could’ve been better chosen, I don’t disagree with you there. _G.PN in particular was pretty bad (although I wasn’t sure what else to call it, as it represents Positive Negative for its function). Though if I do go change the names now, anyone who could already be using those functions are going to be a little upset when it stops working by telling them the function under a certain name doesn’t exist anymore. I’d like to avoid that if at all possible.
I actually wasn’t aware of this. I appreciate you bringing this to my attention, and I will make the corrections in the code.
As for that… that was a little left-over piece from what it was a part of originally, and since it didn’t break anything as long as you used it as instructed, I didn’t bother to change it. Though perhaps I should.
I agree that it should’ve been that way. I just did it the way I did at the time since it was more convenient coding-wise for me. I would change it, but again the same issue that if someone is already using this and if I change it, they’re going to be a little confused as to why it suddenly breaks.
The one thing I’m stuck on is what would make for the best implementation of the module itself in Studio. If I can avoid using the _G. method then that’s good and all, but the require(ID) method only works via normal scripts and not local ones. Should it just be something that you insert somewhere both kinds can find it from? I’d like both methods to be able to auto-update whenever I update the module.
If this isn’t a resource meant for beginners, that gives an even stronger reason for any corrections to be made to fit a production-ready pipeline up to be usable, salvagable, maintainable and so on, if such is the intent. As for the subjective feedback, that’s up for debate, I guess.
Appreciate the clarification to misunderstandings and the response as such.
Do what Roblox does and deprecate the function. Keep PN in and create another slot that references the aforementioned function, effectively creating an alias. Document the alias function as the official and don’t make mention of the one that shouldn’t be used (PN becomes an function that’s recommended not to use). It’s sort of mushy if you assign directly to _G because you have to do a table lookup, but if you have the function created beforehand then that works.
Code examples as follows:
-- Alias
_G.PositiveNegative = _G.PN
-- Pre-existing function
local function positiveNegative(...)
end
_G.PN = positiveNegative
_G.PositiveNegative = positiveNegative
The best implementation of libraries is by not having it usable through require-by-id and instead having it able to be placed in the game hierarchy itself, into a framework or library loader fetch repository or whatever. This is completely my opinion but I believe that requiring by id has absolutely no place in a production environment whatsoever.
Normally if I need an asset to get automatically updated across all places using it or have it be usable across several places, I use packages.
