How can I make this code cleaner?

Help and Feedback Scripting Support
#1 
--[[
	@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
#2

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
#3

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
#4

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

6 Likes
#5

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)
#6

Didn’t even realize that was a thing! Nice

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

1 Like
#7

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
#8

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)
#9

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