Is this a good way to write a function?

Help and Feedback Code Review
#1

I feel like there’s something that could be done to improve this function that or I feel that I may have done something that people ahead of me in experience would try to avoid doing.

Is this a good way to write a function?

local function Start(player,Data)
	local ReplicatedStorage = game:GetService("ReplicatedStorage")
	ReplicatedStorage.SendProfileData:FireClient(player,Data)
	ReplicatedStorage.Cps.Value = Data.Cps
	
	local function SetScreenOrientation()
		local Orientation = Data.Orientation
		if Orientation then
			local ToEnumOrient = Enum.ScreenOrientation[Orientation]
			ReplicatedStorage.SetScreen:FireClient(player,ToEnumOrient)
		end
	end
	
	local function Run() -- The actual clock that handles the money
		local Cash = ReplicatedStorage.Cash
		RunService.Heartbeat:Connect(function(DeltaTime)
			Data.Cash += Data.Cps * DeltaTime
			Cash.Value = Data.Cash
		end)
	end
	
	local function Clicking()
		local Clicked = ReplicatedStorage.Clicked
		Clicked.OnServerEvent:Connect(function()
			Data.Cash += Data.Register
		end)
	end
	
	SetScreenOrientation()
	Run()
	Clicking()
end
5 Likes
#2

Well the 3 inner functions be called again? Or are they just called once at the end of the outer function? If so, the only advantages of doing it like that only has one advantage, which is that the function names comment on what those blocks of code do, and that those logically connected parts of the code have a scope for themselves where local variables can exist. But you don’t need functions to get comments and blocks, you can use comments and blocks for that:

local function Start(player,Data)
	local ReplicatedStorage = game:GetService("ReplicatedStorage")
	ReplicatedStorage.SendProfileData:FireClient(player,Data)
	ReplicatedStorage.Cps.Value = Data.Cps
	
	do -- Set screen oientation
		local Orientation = Data.Orientation
		if Orientation then
			local ToEnumOrient = Enum.ScreenOrientation[Orientation]
			ReplicatedStorage.SetScreen:FireClient(player,ToEnumOrient)
		end
	end
	
	do -- Run the clock that handles the money
		local Cash = ReplicatedStorage.Cash
		RunService.Heartbeat:Connect(function(DeltaTime)
			Data.Cash += Data.Cps * DeltaTime
			Cash.Value = Data.Cash
		end)
	end
	
	do --Clicking
        local Clicked = ReplicatedStorage.Clicked
		Clicked.OnServerEvent:Connect(function()
			Data.Cash += Data.Register
		end)
    end
end

In this case, having the extra blocks doesn’t really help. It does create a visual distinction between logically distinct parts of the code, but just comments on their own can do that as well. Extra blocks has the disadvantage of indenting things further, which can make the code a bit harder to understand and there’s also a limited budget of indentation levels because screens are only so wide:

local function Start(player,Data)
	local ReplicatedStorage = game:GetService("ReplicatedStorage")
	ReplicatedStorage.SendProfileData:FireClient(player,Data)
	ReplicatedStorage.Cps.Value = Data.Cps
	
	-- Set screen oientation
    local Orientation = Data.Orientation
    if Orientation then
        local ToEnumOrient = Enum.ScreenOrientation[Orientation]
        ReplicatedStorage.SetScreen:FireClient(player,ToEnumOrient)
    end
	
    -- Run the clock that handles the money
    local Cash = ReplicatedStorage.Cash
    RunService.Heartbeat:Connect(function(DeltaTime)
        Data.Cash += Data.Cps * DeltaTime
        Cash.Value = Data.Cash
    end)
	
	--Clicking
    local Clicked = ReplicatedStorage.Clicked
    Clicked.OnServerEvent:Connect(function()
        Data.Cash += Data.Register
    end)
end

But if you really like the “lots of little functions that all get called at the end” way of doing it, it’s definitely not bad, just not how I’d do it. It does make it easy to see what happens in what order at a more abstract level, like you can read just

	SetScreenOrientation()
	Run()
	Clicking()

and have a decent idea of what the function as a whole does. Although code blocks can be folded so it’s still easy to get a quick overview of the whole function.

Other than that, you could work on your variable naming. “Run”, “Clicking” and “Start” are bad variable name. “Clicking” is really bad. “SetScreenOrientation” is good. Usually function names should contain a verb that describes what it does. “Clicking” doesn’t have a verb, so it’s very unclear to figure out what it does just from the name. A better name would be “SetupClicking”. “Run” has a verb, but unless you read the comment then it’s still confusing because it’s not specific enough. Sure it runs something, but what? A better name would be “RunClock”, or even better “SetupCashPerSecond”. “Start” is a variable, but it can also be a noun. That makes it ambiguous whether the function starts something, like a process, or if it gets run at the start of the game. I can’t guess what it’s supposed to do from the rest of the script, so I can’t suggest a better name.

With the name changes I suggested, your way of laying things out seems even better to me:

        SetScreenOrientation()
	SetupCashPerSecond()
	SetupClicking()
2 Likes
#3

Actually, I had thought that if i used the do block then the variables would be garbage collected and the next person to join the game wouldn’t be able to use the function(the function’s inside Server Script). I’m guessing that’s not true… I also thought that if I wrote the function first then called it at the bottom then the computer wouldn’t have difficulties reading while trying to run it. Yes, that seems pretty ridiculous for a reason which I assume also is not true.

I don’t really have any trouble reading this and do lua blocks and the 500+ lines of code because I spend like half a day(i’m not kidding) having to look through it while making new other codes. But like you said, writing the functions out like this isn’t bad in a sense that they’re more readable, except for a little deoptimization(if that’s even a word).

Coming up with a name for these functions to summarize what it does was really difficult, so I used imagery words to remember them. I named it Run() because I picture of a person running a lap and stop watches. I named it clicking() because a person is clicking/tapping on a gui button. I’m also probably the only one that will ever mess with these functions if they break… I have not once thought of naming it based on verbs so this will help really help out.

Yes it’s the first function(aside from DataStores whatnot) and most important one that gets started before anything else comes first. It’s the core mechanic of the game is what I’m trying to say. (Sets, sends and runs important game values).

2 Likes