How can I make this code cleaner?

--[[
	@Jamie_Jr
	6/17/18 - Progression #1
	Created for _____
--]]

DataStore = game:GetService("DataStoreService")
ReplicatedStorage,rp = game:GetService("ReplicatedStorage"), game:GetService("ReplicatedStorage") -- rp, ReplicatedStorage
Modules = rp:WaitForChild("Modules") -- Modules, contains important data and events.
Players, plrs = game:GetService("Players"), game:GetService("Players") -- Players, plrs
GenerateObjects = require(Modules:WaitForChild("GenerateObjects"))
CURRENCY_DATA = DataStore:GetDataStore("Currency") -- Currency storage

USER_DATA = {} -- Users data when loaded

DATAKEY = 100 -- If this is changed, all of the data will be reset!

-- Core Code

Players.PlayerAdded:connect(function(plr)
	local Data = plr:GetAsync(plr.UserId..DATAKEY)
	if Data == nil then
		Data = ResetData
	end
	
end)

That is my current code/script… I tried to make it look as clean and neat as possible… yet it still looks messy to me.

Could this be considered nice clean code? If not how should I fix it.

1 Like

Looks fine other than your variables being a bit smashed together at the top and some other nitpicks. You shouldn’t be worrying too much about “Clean code” unless you literally can’t understand what you’ve written.
Also please use local variables.

Nitpicks

connect is deprecated, you should be using Connect.
Also, this is weird.

ReplicatedStorage,rp = game:GetService("ReplicatedStorage"), game:GetService("ReplicatedStorage") -- rp, ReplicatedStorage
Players, plrs = game:GetService("Players"), game:GetService("Players") -- Players, plrs

Why?

3 Likes

Both of the things you mentioned are things I changed to make it look cleaner (Using non-locals, and mashing things together)

It seem’s I should go back to how I was writing haha.

1 Like

The code review category is more suited for a post like this.

6 Likes

It does seem like you are attempting run GetAsync on a Player object

Players.PlayerAdded:connect(function(plr)
	local Data = plr:GetAsync(plr.UserId..DATAKEY) <- plr Is a Player Object, not the datastore :P
	if Data == nil then
		Data = ResetData
	end
	
end)

Didn’t even realize that was a thing! Nice

@The_PieDude The code wasn’t done and I was testing some things.

1 Like

Dunno how others feel about it, but stating two variables that are kinda long in one lone doesn’t look attractive, similar to having one if statement that is wider than the script page on a single line.

ReplicatedStorage,rp = game:GetService(“ReplicatedStorage”), game:GetService(“ReplicatedStorage”) – rp, ReplicatedStorage

It’s turn that into

ReplicatedStorage = game:GetService(“ReplicatedStorage”)
rp = game:GetService(“ReplicatedStorage”)

If not just

ReplicatedStorage = game:GetService(“ReplicatedStorage”)
rp = ReplicatedStorage

1 Like

Is there a reason you need 2 variables with the same value? If so, then I would recommend doing this…

ReplicatedStorage = game:GetService("ReplicatedStorage")
rp = ReplicatedStorage

However, otherwise, just use one variable, like this:

--[[
	@Jamie_Jr
	6/17/18 - Progression #1
	Created for _____
--]]

DataStore = game:GetService("DataStoreService")
ReplicatedStorage = game:GetService("ReplicatedStorage")
Modules = rp:WaitForChild("Modules") -- Modules, contains important data and events.
Players = game:GetService("Players")
GenerateObjects = require(Modules:WaitForChild("GenerateObjects"))
CURRENCY_DATA = DataStore:GetDataStore("Currency") -- Currency storage

USER_DATA = {} -- Users data when loaded

DATAKEY = 100 -- If this is changed, all of the data will be reset!

-- Core Code

Players.PlayerAdded:connect(function(plr)
	local Data = plr:GetAsync(plr.UserId..DATAKEY)
	if Data == nil then
		Data = ResetData
	end
end)

I recommend pcalling your datastores in the rare case you might have reached the limit.