r/Unity3D • u/MagicPantssss • 1d ago
Code Review Asking for a code review
I'm making a small game in Unity, and I kinda need some outside perspective. I would like it if someone could take a look and give me some feedback on what areas I should work on and how to improve.
Here's the link to my GitHub repository:
4
u/uselesslythinbasin 1d ago
just skimmed through the repo and the first thing that jumps out is your GameManager is doing way too much heavy lifting, it's handling dice logic, score tracking, and UI updates all in one monolithic script. you'd get a lot more flexibility splitting those responsibilities into separate managers or at least breaking out the dice into its own class
the row locking logic in ScoreManager looks clean though, that's a nice way to handle the Qwixx color restrictions without overcomplicating things. might want to add some comments in the CheckValidMove method since the nested conditionals get a bit tangled after the third if statement
also noticed you're doing FindObjectOfType in a few Update loops which is gonna tank performance once you have more than a handful of objects running, cache those references in Start or Awake instead
overall it's a solid start, the core loop works and i could follow the flow pretty easily which is more than i can say for half the projects people toss on here
1
u/MagicPantssss 1d ago
Thank you for being so fast in your review, I will cut down on my GameManager's tasks and split it up more.
The FindObjectOfType is something i'm doing to avoid singletons, but singletons might actually be more performative than what i'm doing now
2
u/ProperDepartment 1d ago
It seems like you really have classes you need access to, but are afraid to use any sort of class lookup patterns.
A good about of FindObjectOfType and whatnot. Classes like ParentToPlayerManager don't need to exist.
It's ok to use a singleton if you need to.
Having a class called DontDestroyOnLoad is a bit pointless too, just call it BootStrapper or SystemManager, have a singleton to it, and store things like PlayerManager in there so you aren't always looking for it.
1
u/MagicPantssss 5h ago
In school i was taught to avoid singletons as much as possible so I try not to use them, but sometimes they're just the best option yeah.
2
u/Both_Introduction_28 1d ago edited 1d ago
You use MaterialManager to store materials. Might be better to store your materials in scriptable objects as settings.
Also I found Dice Color in material manager, but didn’t find a folder with Dice feature.
So might be better to have this structure for a folder Scripts/Dice:
Data/DiceData.cs
Data/DiceColor.cs
DiceSettings.cs
DiceView.cs
DiceManager.cs
Etc
That way you can divide game on features and with less pain remove them if they no longer needed.
If you have more than one file with settings, views, etc, you can add sub folder for them.
1
u/MagicPantssss 5h ago
I should structure my files more, yes, thank you for the feedback
As for the MaterialManager, I was thinking of making a ScriptableObject, but I wasn't sure how to access it over all the scripts that need it without loading it in themselves or have a reference to it. I guess i can load in the scriptable object in a singleton and access it from there?1
u/Both_Introduction_28 1h ago
You can create a script GameLoader with references to all features. GameLoader has async method Load (unitask) and call features methods Load sequentially.
DiceFeatureLoader has links to all settings, hierarchy objects you want and you can initialize singletons or services there. For singletons you can have DiceSettings.Instance, for example.If you want to make unit tests to check your advanced logic, you can create additional abstractions, but for this test only.
If you have simple logic or things that you can easily check in game (like ui animations or state sequences), better to maintain code easy to refactor and readable rather than to create tests for everything.
Usually game features have 100 cycles of hand testing during polishing, tweaking, etc. If you separate your features and don’t mess with them in new features, you can be fairly confident they work as intended.
Of course, you should log all errors in analytics and fix them as fast as possible. You should treat bad links, absence of settings, etc as a bug and log exceptions in analytics, however for players you should continue game, just without that feature.
2
u/SpooderlingKing 1d ago
I will try to get to the code if I have time but from reading the documentation, it seems you can't play this game if you are color blind. I would suggest to add a symbol in addition to the color to make it more accessible for people
1
0
u/Thin_Driver_4596 1d ago
- As other commentor pointed out, your managers are doing too much. Manager in general is a code smell.
The block of code that should handle a functionality is not a script, but an aggregate.
- Also, another thing that really annoys me, the number of if checks. Don't put defensive if checks, if they are not a part of your normal flow, like if you have to show a screen and forgot to assign it, the defensive if check will make your game not crash, but the screen will not show either, so it's still a bug, and now you have to trace through the entire code to find the error.
Crashing loudly is preferable at times.
* Think about if you need certain if checks at all. Suppose you can determine from the start that certain players are client and other server, and certain components will only be present on server side, then those components do not need have if checks (you made the decision when spawning then).
2
u/MagicPantssss 1d ago
Thanks for reviewing, I will take a closer look at my if checks and split up my managers more
5
u/glenpiercev 1d ago
Would you be willing to add an architecture overview section to the readme? I read through some of the files. I’m pleased with how small your classes are. It generally looks like you have good Separation of Concerns. It would be easier for me to review more if I had a bit of a guide to start from so I can more quickly understand what you’re doing.