Services are also constants. Would it be weird just capitalizing them too?
local USER_INPUT_SERVICE = game:GetService("UserInputService")
1 Like
Do yourself and future collaborators a favor, teach yourself to write it as
local UserInputService = game:GetService("UserInputService")
General form: local A = game:GetService("A")
Reserve the uppercase-underscore notation for static values that you expect someone might want to tweak or when it leads to better code semantics when they are named instead of anonymous values (i.e. magic values in your code).
6 Likes
Here’s an example of my class implementation btw:
HIERARCHY
WEAPON CLASS
local Weapon = {}
Weapon.__index=Weapon
function Weapon:Equip(modules,Main)
print(self.weptype.." equipped")
modules.clientModules.Gui:ShowWepInfo(Main)
modules.clientModules.Gui:UpdateWepInfo(self,Main)
end
function Weapon:Unequip(trackFile,Gui,Main)
print(self.weptype.. " unequipped!")
Gui:HideWepInfo(Main)
--stop tracks
for i, v in pairs(trackFile) do
v:Stop()
end
end
function Weapon:Sprint(trackFile,modules)
if trackFile.unSprint then
trackFile.unSprint:Stop()
end
trackFile["sprint"]=modules.clientModules.Animate.new("Fire")
trackFile["sprint"]:Play()
end
function Weapon:UnSprint(trackFile,modules)
if trackFile.sprint then
trackFile.sprint:Stop()
end
trackFile["unSprint"]=modules.clientModules.Animate.new("Fire")
trackFile["unSprint"]:Play()
end
function Weapon.new(weptype)
local wep = {
weptype=weptype,
}
setmetatable(wep,Weapon) --inherit all wep functions
return wep
end
return Weapon
FIREARM CLASS (subclass of superclass “Weapon”)
local Weapon = require(script.Parent.Parent)
local Firearm = {}
Firearm.__index=Firearm
setmetatable(Firearm,Weapon)
function Firearm:Fire(trackFile,player,modules,Main)
if self.mag>0 then
if self.fireType == "Semi" then
self.mag=self.mag-1
--print("mag - "..self.mag)
modules.clientModules.Gui:UpdateWepInfo(self,Main)
trackFile["fireTrack"]=modules.clientModules.Animate.new("Fire")
trackFile["fireTrack"]:Play()
end
else
self:Reload(modules.clientModules.Gui,Main)
end
end
function Firearm:Reload(Gui,Main)
if self.mag<self.max and self.pool > 0 then
local needed = self.max-self.mag
local toAdd
if self.pool-needed > 0 then
toAdd=needed
self.pool=self.pool-needed
else
toAdd=self.pool
self.pool=0
end
self.mag=self.mag+toAdd
--print("pool - "..self.pool)
--print("mag - "..self.mag)
Gui:UpdateWepInfo(self,Main)
end
end
function Firearm.new()
print("new")
local firearm = Weapon.new("Firearm")
firearm.pool=100
firearm.max=10
firearm.mag=10
firearm.fireType="Semi"
setmetatable(firearm,Firearm)
return firearm
end
return Firearm
Anything I should fix or “improper”?
1 Like
Just a couple notes.
- For the file structure in explorer, I’d make the subclasses direct children of their parent class, without the intermediate Subclasses file. It’ll make requiring them much less of a pain.
- It is a common convention to start method names with a lowercase character. Classes begin with an Uppercase character. This also helps everyone tell the difference between Roblox userdatas and your classes: Roblox uses Uppercase characters for their method names. Other variables like Gui and Main should always start with lowercase characters.
- Another debate at the Roblox office was the use of [“property”] and .property. Can you guess which I like? Using .property seems clearer to me because it uses less symbols that add to the first-look complexity, but there is a valid argument to sometimes use [“property”]. The difference is based on if the table is being used as a C hash table or C structure. A structure’s structure is, well, known ahead of time so .property should be used. The circumstances when the table is being used as a dictionary and you can use [“property”] instead of [varPropName] are rare, but exist. I argue that Lua isn’t C, as much as I love C, and that Lua designers intentionally made tables be the only method of defining structure without differentiating between arrays, sparse arrays, hash tables, objects, structures and what not.
A quick look over the rest looks good, but I need to go again and will be back in a bit.
2 Likes
I also have modules under the client main script
Animate
local Animate = {}
Animate.__index=Animate
function Animate:Update(dt)
if self.playing then
self.t=math.min(self.max,self.t+dt)
print(self.t)
end
end
function Animate:Play()
self.playing=true
end
function Animate:Pause()
self.playing = false
end
function Animate:Stop()
self.t=self.max
end
function Animate.new(animName)
local animTrack = {
playing=false,
t=0,
max=1,
}
setmetatable(animTrack,Animate)
return animTrack
end
return Animate
Gui
local Gui = {}
--static functions
function Gui:ShowWepInfo(Main)
Main.WepInfo.Visible = true
end
function Gui:HideWepInfo(Main)
Main.WepInfo.Visible = false
end
function Gui:UpdateWepInfo(wep,Main)
if wep.weptype == "Firearm" then
Main.WepInfo.Pool.Text=wep.pool
Main.WepInfo.Mag.Text=wep.mag
else
Main.WepInfo.Pool.Text="-"
Main.WepInfo.Mag.Text="-"
end
end
return Gui
Should I change up this organization?
Some more notes about the Weapon and Firearm classes:
- Whitespace is sacred. There are times when it is important to use, and other when it is inappropriate. Around binary operators (+, ^, %, …), comparisons (<=, ==), assignments (=), lists (item1, item2) it is important to use.
firearm.pool=100should befirearm.pool = 100. These files have mixed use, which is an abomination in the sight of all that is holy! A place that is optional to use is after { and before }. Some think it is easier to read, other are indifferent. - In the weapon class I see that the bane of all programmers’ existence was used. It creates two animations and names both “Fire”, which is clearly not what is should be named. In this case I’d remove the option to name the new animation and in Animate always keep the same name. However in the future when you find yourself copying and pasting snippets of code, stop and consider if you can easily or should put the code into a function. Compositional functions are the functional programmer’s bread and butter. In strict OOP, copying and pasting is a code smell as one of the purposes of inheritance and composition is to eliminate copying and pasting and encourage code reuse. When you do resort to copying and pasting, be VERY careful make sure that you change everything you need to. It is easy to simply skip over and not think about the logic going on when you copy and paste. However, unlike information hiding functions, copying and pasting code doesn’t explicitly list its input. You need to mentally re-evaluate all the code each time you copy and paste.
- For properties inside of classes, don’t prepend the class name. I know it is tempting when
weptypeturns intotypewhich the IDE then highlights as a special function, but look for other ways to name it. In this case, I believeclassNamewould be more appropriate. -
weptypeif it was to be used should also bewepType -
weptype(nowclassName) should be defined by the child, not passed into the parent - To enable full inheritance cleanly, there needs to be a difference between instantiating a class, and the class’s constructor. I see these two are confused in this code (and the example code I gave would do the same, it wasn’t for full inheritance). Instead something like this would be better:
-- Class Weapon
--function Weapon.new()
--...
--end
-- nothing needs to be initialized in a weapon
function Weapon:init()
end
-- Class Firearm
local CYCLE = {
SEMI = 'Semi';
}
local Firearm = {
super = Weapon;
}
Firearm.__index = Firearm
setmetatable(Firearm, Firearm.super)
-- ...
function Firearm:init()
Firearm.super.init(self)
self.pool = 100
self.max = 10
self.mag = 10
self.cycle = CYCLE.SEMI
end
--function Firearm.new()
--...
--end
-- In a file of shared functions
local function new(class, ...)
local obj = {}
class.init(obj, ...)
return setmetatable(obj, class)
end
This prevents both the super and the sub class setting the object’s metatable.
- Also note that I changed the
fireTypetocycleand made a constant table, much like an enumeration. In looking for a better name, wikipedia said this: “Firearms are also categorized by their functioning cycle or “action” which describes its loading, firing, and unloading cycle.” So I took the common word, cycle, to describe that functionality rather than generic fireType. - Many functions here take a table as an argument and only use one field from it. This doesn’t obey the principle of least privilege and is really scary because if fields in a table are mutated in a function, unlike other types of values like numbers or immutable strings, the change also can be seen from the parent. Instead, try to directly pass in the least amount of information possible.
- When arguments like ‘modules’ are needed in many functions, (especially in recursive functions) you should consider making it an upvalue of all of those functions. This often cleans up code significantly. In this case because trackFile actually needs to be the same between all the method calls, I’d actually make it a property assignable by :init(trackFile).
- I like the size of the class methods. They obey the principle of single purpose and are concise.
local toAdd
if self.pool-needed > 0 then
toAdd=needed
self.pool=self.pool-needed
else
toAdd=self.pool
self.pool=0
end
self.mag=self.mag+toAdd
- In the above code, toAdd is only used once and so
self.mag = self.mag + [something]can be inlined andself.pool - needed > 0is mathematically equivilent toself.pool > neededwhich changes produce this:
if self.pool > needed then
self.mag = self.mag + needed
self.pool = self.pool - needed
else
self.mag = self.mag + self.pool
self.pool = 0
end
or since 0 = self.pool - self.pool,
local delta = math.min(needed, self.pool)
self.mag = self.mag + delta
self.pool = self.pool - delta
As for project organization, I recommend putting most client modules in Replicated Storage, but the way you have it is fine. This was a recent topic on the forum: How do you optimize and organize your code?
Hmm. Looking over these posts, I see why good software engineers are paid so much… there really is a lot to it!
3 Likes
Another thing, why is it bad to have a var table?
This is how I deal with the 60 upvalues thing.
What happens if a connected module (my client is broken up into different modules) needs to change a value on the main client code? It would need a table wouldn’t it?
Also why do I need an init method?
Should calling the .new constructor with the necessary parameters to set the variables be enough?
Seems like having an init thingy makes it more complicated.
Using a var table shared in your code is like using the shared or _g table in your code. It tightly binds your functions together and tracing where a value was edited or all the places that can affect a variable can become difficult. In large projects, it becomes impossible. Not using shared state and following the principle of least privilege reduces debugging time and makes creating bugs more difficult.
Separating the initialization of a class from instantiation is required for clean inheritance. For example, when instantiating a child class, both the initialization of the parent and the child must be run, but only one instance is created. I changed the name from new to init to signify the difference.
hm, what do i do if i want to change something from the main code like a variable thats not a table (without a vars table) on a module script connected to that main code??
is it bad practice to have a module script act as an “extension” to your main script??
Information exchange is usually done using arguments and results, function input and output. The main script would create a wholly independent instance of a class, which may in turn create more instances. The main file will then use class methods to manipulate the instance it created, and the instance will return values which are results of that operation which the main script can use with conditional branching to decide what to do next.
Class, as much as possible, should be completely independent and each object should, again for the most part, contain its own state. This help isolate errors and bugs. Most of the effort of good program design is in determining the relationships between information. OOP most of the time can accurately capture the relationships efficiently and create highly compartmentalized programs, but not always. This is one reason why I like Data Oriented Programming rather than Object Oriented Programming. Your project structure will most likely fit into an OOP model, but you may have to do some rejiggering to get it there.
I’d note that Java programs (a language that tried and failed to be fully OOP) do not allow a main method outside of a class. They don’t allow any methods outside of classes. That is a functional language feature, not an OOP one. The more code and data residing in your main function rather than cleanly split out into classes and participating in relationships with other objects, the more you are trying to mash two methods of programming together and difficulty you will have.
ECS (entity-component-system) frameworks are designed to help large projects organize their code and reduce dependencies. When I used my ECS implementation in a project, the main file simply loaded all the components and systems into the framework, then created the first entity, applied the Main assembly to that entity, and finally began the update loop (calling ECS.update() after every wait()). It was a very simple file. All of the program’s logic was contained inside of the framework.
By “extension” I think your implying tightly connected and sharing lots of information. Not all of the time. Sometimes with OOP, creating large classes is the only way handle complex relationships. When this happens, it is desirable to split the class’s implementation into multiple files. Is this optimal? No. Optimally all of your files will by 70 - 80 lines (about two screens worth) of code and contain simple logic. None of the files would have duplicate logic. Information flow through the program would be simple and predictable. But alas, the optimal program is ever elusive to most program designers.
1 Like
One last thing about folder organization. I have a module with methods but they’re all static. There’s no object creation in them. What folder name would these type of modules go under? ‘Services’??
A collection of functions is called a library. Lua naturally has the string, table, math, and other libraries built in. A common custom library name is ‘util’, short for utility, and contains lots of commonly used functions.
On a side note, OOP classes have methods (in Lua, functions called with ‘:’) and methods are functions with a ‘this’ or ‘self’ bound onto it. Pure functional programs don’t have methods and pure OO programs don’t have functions.
So let’s say for my Gui Util, which is organized like this:
(tell me if there’s anything wrong with this btw)
Here’s the code:
--constants
local PLAYER = game.Players.LocalPlayer
local PLAYERGUI = PLAYER:WaitForChild("PlayerGui")
local MAIN = PLAYERGUI:WaitForChild("Main")
local Gui = {}
--static functions
function Gui:ShowWepInfo()
MAIN.WepInfo.Visible = true
end
function Gui:HideWepInfo()
MAIN.WepInfo.Visible = false
end
function Gui:UpdateWepInfo(wep)
if wep.type == "Firearm" then
MAIN.WepInfo.Pool.Text=wep.pool
MAIN.WepInfo.Mag.Text=wep.mag
else
MAIN.WepInfo.Pool.Text="-"
MAIN.WepInfo.Mag.Text="-"
end
end
return Gui
If I want to make it so you click Main.Button would I write that function directly into the Gui module code?
OR would I write it in my main code and call a static method on Gui?
For example:
--main code
--constants
local PLAYER = game.Players.LocalPlayer
local PLAYERGUI = PLAYER:WaitForChild("PlayerGui")
local MAIN = PLAYERGUI:WaitForChild("Main")
local MODULES = ReplicatedStorage:WaitForChild("Modules")
local UTIL = MODULES:WaitForChild("Util")
local GUI = require(UTIL:WaitForChild("Gui"))
MAIN.Button.MouseButton1Click:connect(function()
GUI:SomeMethod()
end)
Which way is better practice and the proper way to do it??
You have an interesting problem on your hands. But first, some feedback.
- So… I may have been a bit excessive when I said every constant value that shouldn’t change should be in uppercase. I like the way @buildthomas said it:
He said there are two situation in which you should use uppercase variables:
- Values someone may want to tweak in the future
- To put a label on anonymous magic values
Variables that commonly fit these definitions are usually being used as constants being passed to functions / instances outside your code. Many times these are ‘configurable’ values. Values used as enumerations and UDims are also commonly put at the top of the file. Under this new definition, MAIN and PLAYER_GUI do not meet the criteria for being capitalized. Sorry about that. 
-
A couple small things. I still see assignments without spaces. Also I corrected this in my one of the copies of your code above hoping you would see it, but
:connectis now:Connectat least in part to conform with standard Roblox capitalization standards. The old method is deprecated. I’d consider renaming Gui to be more specific like WeaponGui. If it really is a class dealing with everything to do with the gui, then it should be broken down a bit. I saw that you renamedweptype.
-
Who owns the button connection code depends on who is interested in the button’s behavior. How much is the GUI class in charge of? If it is all GUI code, then it belongs in the GUI class. But then the question becomes, where to put it in the GUI class? I’d bet this is the actual question that made you post. The problem is that you’ve fallen into a common anti-pattern some programmers think is beneficial (but don’t believe them!) and are having troubles because of it. I see the enticement of making a singleton for a GUI that is only created once per a client, but it really should be a class with only one instance. OOP isn’t about being efficient (again, a reason I detest OOP) and naturally has overhead to obey its design decisions (basic getters and setters, single instance instead of static, and other things. But I digress.
Wikipedia says this about singletons in general
But I say it is even more so in OOP. Static methods and classes are also often considered anti-patterns (see Anti-Patterns and Worst Practices). This includes Utility classes. Consider this article instead:
OOP alternative to Utility Classes They are all features of functional programming and have snuck into OOP languages like Java to taint them.
Now, that being said, you can use static methods and classes. Some think singletons are a good pattern. Where will you put the connection for the button? I’m not sure, as code outside of classes doesn’t belong in a class file but then again main shouldn’t have its nose is every other modules’ business. If you were to make GUI a class instead of a singleton, it would neatly fit into the constructor, and the PlayerGui.Main would be passed in as a constructor argument and become a property. If you choose to make it a static class, I’d remove the ‘:’ because those functions are not using self. They are not methods. (Yes, I said functions do not exist in OOP, and I stand by that). I’d recommend going on a static class refactoring spree with the knowledge in the articles above to make it all OOP.
Or, you know, decide to join me in a campaign against OOP, the disgusting paradigm that for some reason is claimed to be natural to programmers and clean? We’ve clearly seen that functional programming is more natural and ingrained to every programmer that even the designers of THE OOP language let it sneak in! It doesn’t match the data and operations, thrashes CPU caches, destroys locality of reference, and leaves anti-patterns and code smells in any actual project’s wake? Its bad for the programmers and bad for the machines. There is a reason Java VMs are written in C and C++. Its because while they may offer OOP support, the still have some sanity left in them. Yes, it would be a glorious campaign. (I’m sorry, I had to let it out…)
1 Like
Just to make it clear, you did say its good practice to keep things independent of each other? Especially my custom made classes?
Bc I did a lot of revisions to the code since the first post.
1 Like
Yes, in all cases you want code to be compartmentalized, and independent. Classes in OOP and functions are meant to hide details from their creator/callers, not simply contain operations. They simplify their users’ lives. The principle of least privilege isn’t just to prevent nefarious users from taking advantage of information (like the creation of anti-patterns) but also to prevent simple bugs and ease the strain on a developer’s mind. They shouldn’t have to worry about the whole program when working on a little part of it. Most of us arn’t on top of our game 100% of the time.
You know, when I worked at Roblox, I think people dreaded having their code reviewed by me…
Ok, bc I changed my weapon class from this:
OLD
--services
local ReplicatedStorage = game:GetService("ReplicatedStorage")
--constants
local PLAYER = game.Players.LocalPlayer
local PLAYERGUI = PLAYER:WaitForChild("PlayerGui")
local MAIN = PLAYERGUI:WaitForChild("Main")
local MODULES = ReplicatedStorage:WaitForChild("Modules")
--services
local SERVICES = MODULES:WaitForChild("Services")
local GUI = require(SERVICES:WaitForChild("Gui"))
local Weapon = {}
Weapon.__index=Weapon
function Weapon:Equip()
print(self.type.." equipped")
GUI:ShowWepInfo(MAIN)
GUI:UpdateWepInfo(self,MAIN)
end
function Weapon:Unequip()
print(self.type.. " unequipped!")
GUI:HideWepInfo(MAIN)
--stop tracks
for i, v in pairs(self.trackFile) do
v:Stop()
end
end
function Weapon.new()
local wep = {equipped=false}
setmetatable(wep,Weapon) --inherit all wep functions
return wep
end
return Weapon
to this
NEW
local Weapon = {}
Weapon.__index=Weapon
function Weapon:Equip()
print(self.type.." equipped")
self.equipped = true
end
function Weapon:Unequip()
print(self.type.. " unequipped!")
self.equipped = false
end
function Weapon.new()
local wep = {equipped=false}
setmetatable(wep,Weapon) --inherit all wep functions
return wep
end
return Weapon
Is this what you mean when you say least privilege and independent? Am I doing it right?
Ah, I see. It does look good to me. You’re doing it right! But it does make me wonder, how are the GUI interactions and animations being controlled now? If you are still looking for a good way to do this and haven’t done so already, I’d recommend making a Player class. That class, which creates and maintains the PlayerGui, Character, and Backpack related classes should contain a unequip method since equipping deals with scripts from all three of those areas of responsibility. In OOP you yoink code up higher in the hierarchy to deal with access issues like this. Unfortunately this is also another reason I don’t like OOP as it creates heavy top-level classes.
I’m beginning to think you have a vendetta against spaces. You can’t just backspace everything you don’t like! lol
(Interesting, in the quote above, two periods show up as three…)
lol i didn’t do anything about the spaces yet but ill get to them eventually
GUI interactions and animations are being controlled by the main code now, for example:
--unequip current, if
if equipped then
weps[equipped]:Unequip()
--stop tracks
for i, v in pairs(tracks[PLAYER]) do
v:Stop()
end
end
--equip
equipped = inputObj.KeyCode.Value-48
weps[equipped]:Equip()
--update gui
GUI:ShowWepInfo(MAIN)
GUI:UpdateWepInfo(weps[equipped],MAIN)
Would my Gui module be considered a singleton. If so, whats the single object under that class? I’m still have trouble classifying what type of module it is. And I’m still unclear if it should be an extension of my main script for not.
Also, is it bad having a single main script to control everything?