Feedback on my Player Value creation system

Help and Feedback Code Review
#1

Hi, I’m Mega

I thought of this way to create Player Values which seems quite efficient and fast to me. If I wanted to create alot of values inside a Player that included Pets, Trails, Weapons, OtherStuff having alot of Instance.new() would create a mess and would be very wordy. Any feedback would be appreciated.

Module Script in a Folder in ServerScriptService

local util = {}

function util:Create(insClass, insName, insParent)
    local ins = Instance.new(insClass)
    ins.Name = insName
    ins.Parent = insParent

   return ins
end

function util:MultiCreate(insClass, insParent, ...)
    for _, name in ipairs({...}) do
        util:Create(insClass, name, insParent)
    end
end

return util

A Server Script in ServerScriptService

local Utility = require(script.Parent:WaitForChild("Modules").Utility)
local Players = game:GetService("Players")

Players.PlayerAdded:Connect(function(player)
    local Pets = Utility:Create("Folder", "Pets", player)
    local Trails = Utility:Create("Folder", "Trails", player)
    local Weapons = Utility:Create("Folder", "Weapons", player)
    local OtherData = Utility:Create("Folder", "OtherData", player)

    --//PETS//
    Utility:MultiCreate("BoolValue", Pets, "Red", "Blue", "Orange", "Pink", "Green", "Yellow")
    --//TRAILS//
    Utility:MultiCreate("BoolValue", Trails, "Red", "Blue", "Orange", "Pink", "Green", "Yellow")
    --//WEAPONS//
    Utility:MultiCreate("BoolValue", Weapons, "LightSaber", "Sword", "Pistol")
    --//OTHER-DATA//
    Utility:MultiCreate("IntValue", OtherData, "Coins", "Cash", "Kills", "Deaths", "Wins")
end)

If you are wondering about default values, I add them after initializing my DataStores.
Also I am completely aware of Module Datastorage but this post is not related to that.

4 Likes
#2

Nice work dude. I am sure this will help others too.

1 Like
#3

You could maybe change it up a little with OOP. This Library is really good to use. Also for Roblox Lua.

For Example you could add that they can Change a Value by doing Value:Change(Different).

Even though its well made.

1 Like
#4

Be careful! While this layer of abstraction might seem really nice in the moment, I assure you it’s not!

While meta-programming and utilities might save you some time and keystrokes, explicit code is better than implicit abstractions. For instance, if the developer consuming this snippet uses the exposed Create function, and modifies any property besides Name and Parent, the function was sort of a bust. Furthermore, indexing properties from any of the constructed children created through MultiCreate can’t be accessed as you don’t preserve the reference! It is very unlikely the developer will not need a reference to the item.

Again, while your example use-case does look “cleaner” to a degree, it’s very unrealistic. Most code doesn’t say “create all these values, with the default value, and we don’t actually care about the value”. Even assuming this is initialized through datastores, because you don’t maintain a reference, it informs me that your references are assuming these instances exist or exhaustively checking them. Either way, I doubt it’s worth the developer ergonomics.

1 Like
Better alternative to WaitForChild
#5

Your response seems very informative but can you clarify some points?

I don’t really understand what you mean by this because I don’t need to set any other properties except for the name and parent for Folders or Values.

My DataStores Get all the data in a dictionary form, then loop through the Player’s directories and set the corresponding value if it exists, if not it would set the default value for the ValueBase that exists in another dictionary. This way I don’t need the references at all. The checks you were talking about exist too.

Anyway, Thank You for your feedback, I appreciate it. :smile:

1 Like
#6

Sure thing! As for the topics of Create, consider the following snippet:

local part = Instance.new("Part");
part.Color = Color.fromRGB(255, 255, 255);
part.Position = Vector3.new(0, 10, 0);
part.Parent = workspace;

This is pretty straightforward, and I have not a single doubt you know all of this. This isn’t me patronizing you, it’s just displaying a baseline. However, say we refactor our code base to include Create. The same code will become the following:

local ServerScriptService = game:GetService("ServerScriptService");
local PrototypeUtility = require(ServerScriptService.Modules.Utility);

local part = PrototypeUtility:Create("Part", "we don't really care", workspace);
part.Color = Color.fromRGB(255, 255, 255);
part.Position = Vector3.new(0, 10, 0);

This is fine. It’s not a breaking change, and so what if we never use Name? We can make it an optional parameter or just make it the same as the instance’s class name, etc. Even better, why use Create here? It’s outside the scope of this use-case. Again, this is fine. So far, to most, there is no issue. However, to most people who have either worked in the realm of FOSS or with a team of people (hell, even coming back to a large project a few months later), this is the seed for a really big issue: inconsistency. Do we use Create? Do we not? What does Create do here? Does it have side effects? Do we create more alias functions?

In most cases, this might seem like a slippery slope. Easily dismissable. But I assure you, there’s a reason why large FOSS projects lead by FAANG companies move away from small utility microservices like that to more explicit code. Because at some point, they figured it would be a cute addition and at some point it caused a very large disruption in the project as a whole.

In cases like this, it’s best to combat software entropy by eliminating all the extraneous ways of doing things. It’s safe to say this service (in terms of Create) is extraneous.

Moving right along, let’s look at MultiCreate:

This falls to a lot of the same errors, unpredictability, and inconsistent issues brought up by Create. To me, this is already enough to dissuade its usage entirely.

In a perfect world, these are the steps data storage uses:

  • Data is initialized to the value instances created on site.
  • References to these instances are maintained for usage with autosaving and saving when the player leaves or the server closes.

This being the case, if your code is already dynamic, why hard code the creation of the value instances like shown in your example? It seems counter-intuitive to hard code the names of each value only to dynamically load them?

I can’t stress enough that small services like these, that solve an immediate issue or prevent a line or two, seem really great in the moment but end up being okay, lack-luster, or needlessly inconsistent.

While these issues are almost null and void in an individual project, it’s a good idea to make a project of any size as maintainable as possible.

Also, as an aside, thank you for actually following up! If you have further questions, either reply to me here or message me directly on the devforums!

1 Like
#7

Thank you, this really helped me alot!

1 Like