-
Notifications
You must be signed in to change notification settings - Fork 0
Initial Review #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: empty
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some general comments that I saw in multiple places:
- A lot of the review is "why is this code here?". Code responsibility is a concept that takes a lot of practice to master.
- There's an overuse of
public
, and not just inMonoBehaviour
s. Unity's components often get a pass for having a lot of public variables, because that's how a lot of resources online usepublic
to have it show up in the inspector. But you can also use[SerializeField] private
to accomplish that, while maintaining privacy. In general, default toprivate
and then use public properties if you need read-only access in other classes. If you are trying to modify data from outside the class, you're probably doing something wrong. - It looks like there's a lot of common network code both in
ClientRef
andGameServer
. Because of that, it's hard to follow who is responsible for what code. Consider grouping client and server code into their own namespaces so it's not so easy to cross that line. - Don't use
GetComponent()
more than absolutely necessary. It's an expensive operation. Avoid calling it per frame or in a loop, and cache any calls you make for reuse.
Vector3 playerPos = _client.player.transform.position; | ||
Vector3 _direction = playerPos - transform.position; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you have a mix of variables prefixed with underscores and others without, which throws me off as a reader. A common C# coding convention is to use an underscore to denote a private member variable. I would either forego the underscores or only use them on private member variables.
|
||
class AIController : MonoBehaviour | ||
{ | ||
CharacterController controller; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would always add an access modifier to every variable for consistency and readability.
This PR compares the current state of the repo as of 10PM 12/18/21 and compares it to an empty branch with no code, so that I am able to easily make comments for a repo review. Do NOT merge this PR.