Any way to make this cleaner?

[IAmPinleon (Pinleon)]

local dataStoreService = game:GetService("DataStoreService")
local dataStore = dataStoreService:GetDataStore("TimeFirstJoined#1")
local dataStore2 = dataStoreService:GetDataStore("JoinedOnAnniversary#1") -- number indicates which anniversary
local dataStore3 = dataStoreService:GetDataStore("JoinedOnAnniversary#2") -- number indicates which anniversary

local tagsTable = {}

local tags = game.ReplicatedStorage.Tags
local nameTag = tags.NameTag
local title = tags.TItle
local owner = tags.Owner
local anniversary = tags.Anniversary
local anniversary2 = tags.Anniversary2

local function addTag(object, plr, renameString)
	local char = plr.Character or plr.CharacterAdded:Wait()
	if char then
		local head = char:WaitForChild("Head")
		if head then
			local newTag = object:Clone()
			newTag.Parent = head
			if renameString then
				local text = newTag:FindFirstChildOfClass("TextLabel")
				text.Text = renameString
			elseif renameString == nil then
				--nothing happens
			end
		end
	end
end

local function findTag(tagName, plr)
	local char = plr.Character or plr.CharacterAdded:Wait()
	if char then
		local head = char:WaitForChild("Head")
		if head then
			return head:WaitForChild(tagName)
		end
	end
end

local function adjustTags(originalTag, plr)
	local tagToAdd = findTag(originalTag.Name, plr)
	local tab = tagsTable[plr]
	table.insert(tab, tagToAdd)
	local count = 0
	for _, object in pairs(tab) do
		count = count + 1
		local ypos = 3.15 + ((count - 1) * 0.5)
		object.StudsOffset = Vector3.new(0, ypos, 0)
	end
end

game.Players.PlayerAdded:Connect(function(plr)
	tagsTable[plr] = {}
	addTag(nameTag, plr, plr.Name)
	
	if plr.UserId == 1003594243 then
		addTag(owner, plr)
		adjustTags(owner, plr)
	end
	-----------
	local data = dataStore:GetAsync(plr.UserId)
	
	local START = os.time {year = 2020; month = 6; day = 20}
	
	if not data then
		dataStore:SetAsync(plr.UserId, os.time())
		
		wait(1)
		data = dataStore:GetAsync(plr.UserId)

		if data <= START then
   			print(plr.Name.." joined before Summer 2020")
			addTag(title, plr)
			adjustTags(title, plr)
		end
	elseif data then
		if data <= START then
   			print(plr.Name.." joined before Summer 2020")
			addTag(title, plr)
			adjustTags(title, plr)
		end
	end
	-----------
	local secondData = nil
	secondData = dataStore2:GetAsync(plr.UserId)
	
	local dayNeededStart = os.time {year = 2020; month = 8; day = 14; hour = 1}
	local dayNeededEnd = os.time {year = 2020; month = 8; day = 14; hour = 24}
	
	local currentTime = os.time()
	
	if (currentTime >= dayNeededStart) and (currentTime <= dayNeededEnd) then
		if secondData == nil then
			dataStore2:SetAsync(plr.UserId, os.time())
			addTag(anniversary, plr)
			adjustTags(anniversary, plr)
		end
	end
	
	if secondData ~= nil then
		addTag(anniversary, plr)
		adjustTags(anniversary, plr)
	elseif secondData == nil then
		warn("SOMETHING WENT WRONG")
	end
	-----------
	local thirdData = nil
	thirdData = dataStore3:GetAsync(plr.UserId)
	
	local dayNeededStart1 = os.time {year = 2021; month = 8; day = 14; hour = 1}
	local dayNeededEnd1 = os.time {year = 2021; month = 8; day = 14; hour = 24}
	
	local currentTime1 = os.time()
	
	if (currentTime1 >= dayNeededStart1) and (currentTime1 <= dayNeededEnd1) then
		if secondData == nil then
			dataStore3:SetAsync(plr.UserId, os.time())
			addTag(anniversary2, plr)
			adjustTags(anniversary2, plr)
		end
	end
	
	if thirdData ~= nil then
		addTag(anniversary2, plr)
		adjustTags(anniversary2, plr)
	elseif thirdData == nil then
		warn("SOMETHING WENT WRONG")
	end
end)

This is my script and it is very long, so I was wondering how I could shorten it, or make it cleaner, or more efficient.

So should it be

• More clean
• More efficient
• Both

Also, how good is it?

• 1
• 2
• 3
• 4
• 5
• 6
• 7
• 8
• 9
• 10

Thanks!

[TheEdgyDev]

Could you give a semantic overview of what the end-goal of this file is? Perhaps give some insight on what your game Explorer looks like? Just some context to make review less time-consuming.

Mainly, what are “tags” in this context?

[IAmPinleon (Pinleon)]

So the tags are titles that the player can earn by doing a certain thing.

[TheEdgyDev]

And can you show us what game.ReplicatedStorage.Tags looks like?

[IAmPinleon (Pinleon)]

[TheEdgyDev]

And could you expand one to show us the common shape of what a “Tag” contains?

It would also help to see one or two in-game.

[IAmPinleon (Pinleon)]

This is what it looks like.

What I feel like makes the script very long is the part with the anniversary titles. I basically just copy and pasted the code.

[IAmPinleon (Pinleon)]

You can find the game here and see for yourself:
https://www.roblox.com/games/3656132490/TITLES-Chocoblox-Factory-Tycoon?refPageId=c50aba17-8504-49d1-a267-91301fdb3802

[TheEdgyDev]

So, I’ve created a rendition of your script that, I believe, is more readable and more effective, without deviating heavily from your original design.

You can not just copy and paste this snippet assuming it will work. I’ve made changes on what I believe to be the ideal design not only of your script, but for elements outside of it. This snippet could be adapted, but I’d prefer you use it as a reference when refactoring your code.

Another thing I frequently say is that this is not a prescriptive solution. This is using my own style while keeping it only within one script for the sake of readability on these forums. I’d recommend, in production, your code is more modular than this.

I’d also recommend, in production, further documentation of what each function does and the parameter types / return types. For the sake of brevity, these have been omitted.

local DataStoreService = game:GetService("DataStoreService");
local Players = game:GetService("Players");
local ReplicatedStorage = game:GetService("ReplicatedStorage");

local OG_TIME_RANGE = NumberRange.new(
    0,
    os.time({
        year = 2020,
        month = 6,
        day = 20,
    })
);
local FIRST_ANNIVERSARY_TIME_RANGE = NumberRange.new(
    os.time({
        year = 2020,
        month = 8,
        day = 14,
        hour = 0,
    }),
    os.time({
        year = 2020,
        month = 8,
        day = 15,
        hour = 0,
    })
);
local SECOND_ANNIVERSARY_TIME_RANGE = NumberRange.new(
    os.time({
        year = 2021,
        month = 8,
        day = 14,
        hour = 0,
    }),
    os.time({
        year = 2021,
        month = 8,
        day = 15,
        hour = 0,
    })
);

-- Configurations for how we should handle data store failures.
local MAX_RETRIES = 3;
local RETRY_WAIT_TIME_IN_SECONDS = 20;

local OWNER_USER_ID = 1003594243;

-- Configurations for how the titles should be rendered.
local Y_OFFSET_OF_TITLES_FROM_HEAD_IN_STUDS = 3.15;
local Y_PADDING_BETWEEN_TITLES_IN_STUDS = 0.5;

-- Enumeration storing all of the template titles for easy access.
local TemplateTitle = {
    NAME = ReplicatedStorage.Titles.NameTitle,
    OG = ReplicatedStorage.Titles.OGTitle,
    OWNER = ReplicatedStorage.Titles.Owner,
    FIRST_ANNIVERSARY = ReplicatedStorage.Titles.FirstAnniversary,
    SECOND_ANNIVERSARY = ReplicatedStorage.Titles.SecondAnniversary,
};

-- Data stores in use. Every one stores only a single boolean per each player.
local joinedBeforeSummer2020Store = DataStoreService:GetDataStore("JoinedBeforeSummer2020");
local joinedOnFirstAnniversaryStore = DataStoreService:GetDataStore("JoinedOnFirstAnniversary");
local joinedOnSecondAnniversaryStore = DataStoreService:GetDataStore("JoinedOnSecondAnniversary");

local function isElligibleForOwnerTitle(player)
    return player.UserId == OWNER_USER_ID;
end

local function isElligibleForTimeBasedTitle(player, numberRange, dataStore)
    local alreadyElligible = dataStore:GetAsync(player.UserId);

    if alreadyElligible then
        return true
    end

    local currentTime = os.time();

    local isElligible = (
        numberRange.Min <= currentTime and
        currentTime < numberRange.Max
    );

    dataStore:SetAsync(player.UserId, isElligible);

    return isElligible;
end

local function initializeTitles(player)
    local character = player.Character or player.CharacterAdded:Wait();
    local previousTitlesGameObject = character.Head.Titles;

    if previousTitlesGameObject then
        previousTitlesGameObject:Destroy();
    end

    local titlesGameObject = Instance.new("Folder", character.Head);
    return titlesGameObject;
end

local function setTitles(player, templateTitles)
    local titles = {};
    local titlesGameObject = initializeTitles(player);

    local nameTitle = TemplateTitle.NAME:Clone();
    nameTitle.Name.Text = player.Name;
    nameTitle.Parent = titlesGameObject;

    table.insert(titles, nameTitle);

    for _, templateTitle in ipairs(templateTitles) do
        local title = templateTitle:Clone();
        title.Parent = titlesGameObject;
        table.insert(titles, title);
    end

    for index, title in ipairs(titles) do
        local paddingCount = index - 1;
        local yOffsetPadding = paddingCount * Y_PADDING_BETWEEN_TITLES_IN_STUDS;
        local yOffset = Y_OFFSET_OF_TITLES_FROM_HEAD_IN_STUDS + yOffsetPadding;
        local titleOffset = Vector3.new(0, yOffset, 0);
        title.StudsOffset = titleOffset;
    end
end

local function assignTitlesAndUpdateStores(player)
    local templateTitles = {};

    if isElligibleForOwnerTitle(player) then
        table.insert(templateTitles, TemplateTitle.OWNER);
    end

    if isElligibleForTimeBasedTitle(player, OG_TIME_RANGE, joinedBeforeSummer2020Store) then
        table.insert(templateTitles, TemplateTitle.OG);
    end

    if isElligibleForTimeBasedTitle(
        player,
        FIRST_ANNIVERSARY_TIME_RANGE,
        joinedOnFirstAnniversaryStore
    ) then
        table.insert(templateTitles, TemplateTitle.FIRST_ANNIVERSARY);
    end

    if isElligibleForTimeBasedTitle(
        player,
        SECOND_ANNIVERSARY_TIME_RANGE,
        joinedOnSecondAnniversaryStore
    ) then
        table.insert(templateTitles, TemplateTitle.SECOND_ANNIVERSARY);
    end

    setTitles(player, templateTitles);
end

local function onPlayerAdded(player)
    for try = 1, MAX_RETRIES do
        local ok, errorMessage = pcall(assignTitlesAndUpdateStores, player);

        if not ok then
            local messageOne = string.format(
                "An error has occured while managing titles for player %q: %q",
                player.Name,
                errorMessage
            );
            local messageTwo = string.format(
                "Backing off %d seconds before retry",
                RETRY_WAIT_TIME_IN_SECONDS
            );

            if try < MAX_RETRIES then
                local message = ("%s\n%s"):format(messageOne, messageTwo);
                warn(message)
                wait(RETRY_WAIT_TIME_IN_SECONDS);
            else
                error(messageOne)
            end
        else
            break;
        end
    end
end

Players.PlayerAdded:Connect(onPlayerAdded);

As always, if you have any questions, feel free to ask!

[oskxzr (DULA_PEEP)]

Personally I just add a bunch of comments thorugh my script and it looks tidier.

Adding sections through my scripts is mostly what I do;

 --// Variables

--// Functions

[TheEdgyDev]

I tend to stay away from comment headers.

Line comments explaining why something is done a certain way, explaining a specific edge case , or leaving “TODO” comments I find exceptable.

Comments using some documentation standard (LuaDoc, LDoc, EmmyLua, or something like JSDoc even) I heavily encourage whether that’s for function annotation, variable annotation, or module annotation.

However, comment headers, comments explaining what the code is doing, and the “more-like-ASCII-art-than-anything-informational” comments (you know the ones) are, in my opinion, a greater code smell – no different than more than one newline character or inconsistent casing.

I’m not saying that preference is entirely wrong although if I tried to merge a PR with those riddled about the maintainers, as a single entity, would end me.

[oskxzr (DULA_PEEP)]

Yes. What I was meaning was that in addition to comments explaining why something was done like it is, he should try using headers as well, considering that he already had explaining comments dotted through the script.