I feel that I’m not using object-oriented programming to its best potential—such as I’m not tapping into its most fundamental benefits. What are some techniques that I can utilize to more effectively use OOP principles? And how can I tidy up and clarify my code to make it easier to maintain and less overwhelming? Can you give me specific steps on how to structure my code in a systematic way? Thank you in advance.
Place.rbxl (73.9 KB)
alright. lets go through this, and give it a review.
oki, i gave the place a look, didnt playtest, the code is odd. So, heres what i think (take with a grain of salt, im no pro)
firstly, just looking at item stuff, you mentioned organization. store the items in one table, since from what i saw they are mainly two values (damage, cooldown). using multiple modules is only if each weapon behaves uniquely. and to get into it, you mentioned OOP. to briefly brush it here, if you had a different module, it would be a class inheriting from a broader Weapon class.
for structure, the ItemModule seems to be a crutch here. it does saving, items (more on that), and absolutely lacks a structure (sorry). you need to organize it. separate variables by importance.
a rule of thumb i do
services
modules
(player, special exception)
events
constants
variables
and your functions need separation.
Instead of doing
local FindInHotBarStatus = function(plr, LayoutOrder)
for i,v in ipairs(PlayerHotBarStatus[plr.UserId]) do
if v == LayoutOrder then
return true
end
end
return false
end
where it is global, a better “formatting” is just to do ModuleName._FunctionName()
the _ is basically telling everyone “Hey, this is intended for use in the module only”> it lets you maintain formatting but tell others “dont do this outside of the module”
– code review here
i mostly spoke about structure, and some small things. the biggest issue is inefficiency:
i spotted firstly, why do you do
local ItemModule = require(game.ServerScriptService.Modules.ItemModule)
game.ReplicatedStorage.Equip.OnServerEvent:Connect(function(plr, LayoutOrder)
local item = ItemModule.New(plr, LayoutOrder)
item:Equip(plr)
end)
The problem is that every time they equip, you create a new item, and even though you account for it in :Equip(), you create new items each time, which, is semi useless and wasteful.
Cleaning up, it is nonexistent. the hotbar cache and item cache arent wired (by removecache) to playerremoving, so it also creates memory leak and problem when rejoining, via duplicates.
The client script, I only spotted surface level stuff (as I didn’t debug), but it is worse off than the server. I do not mean to be offensive, sorry… but the structure makes it hard to follow at times.
The names are random, do not follow usual conventions (camelCase, PascalCase, etc) and are sometimes one letter. The code also misuses pairs() (intended for dictionary, client code uses it on something guaranteeing an array), and the condition here
if UI.Enabled == false then
UI.Enabled = true
else
UI.Enabled = false
end
could just invert it.
UI.Enabled = not UI.Enabled
other than that, I am a little tired from the day so I just looked at what I could.
General consensus
I honestly think this isn’t that bad, it just suffers from lack of structuring. OOP could be better implemented, and please stick to one naming convention (it makes the biggest difference). efficiency wise, I laid out all I could read in my current state and with no playtests, so obviously, take my reccomendations (and please tell me where I may have got something wrong) lightly.
thx bye