#can someone look at my code and give me

1 messages · Page 1 of 1 (latest)

mighty radish
#

Id appreciate any serious input.

tardy cloak
#

Please share the code to review

mighty radish
tardy cloak
#

Oh boy

woeful spire
#

they have a cool little bot to show how code is preferred to be posted

#

!code

fathom latchBOT
tardy cloak
#

Consider using a paste site

mighty radish
tardy cloak
#

First of all, do not use public fields. This is something you should never do because there is no clear indication who can modify the value

#

Instead make it private, and either

  • Add a private method that validates it
  • Make it a property with a public setter that validates it
  • Make it a private field with [SerializeField]
#

Considering these look like a bunch of configurable fields for in the editor, make it a private field and add [SerializeField] to expose it in the editor.

#

Just make sure to never use public fields. If a field should be grabbed by another class then make it a property with a public getter and private setter. Always put as little accessibility to data in a class until it is required

mighty radish
#

okay iv seen the SerializeField but never used it yet but ill do that

tardy cloak
#

Second point, when using private fields they start with an underscore. Assuming public Rigidbody rigidbodySelf; takes the component from the editor you end up with [SerializeField] Rigidbody _rigidbodySelf;

#

This is not just because .NET conventions say so, it makes it very clear what the accessibility of the field is, but also the scope of it.

#

You should only have a name with no underscore and camelcase if it's a method parameter, so you know where you can use it

#

It's that little bit of consistency that makes programming way easier

#

Anyway that's mostly it for the variables, I haven't even scrolled down yet

#

You got a Start method with a lot of GetComponent calls but you should put this in an Awake method

#

Awake is for initialization, start is for start of actual behaviour

#

So

 if (!sSInfo.player)
        {
            StartCoroutine(CheckIfObjInSightCoroutine());
        }

Should be in a Start method for example

#

Just puts a hard restriction so these components are required, but it does also make them if they don't exist which can be annoying

mighty radish
#

i appreciate the tips so far. but thats just the variables. im sure the rest of the code is worst.

#

should a character controller script be having over 100 variables or am i doing something deeply wrong.

gaunt barn
#

The underscore is just naming convention that some people follow. You don't have to start following of you don't want to. As long as you keep your code consistent.

gaunt barn
#

As well as a lot of fields that is not clear what they're responsible for.

#

First thing I'd do is separate the movement logic from ai logic. Ai doesn't belong in a movement controller.

mighty radish
tardy cloak
#

It's not that bad to have a ton, you should just keep it as simple as possible for what you need

#

You can separate them in different classes if you want maybe, create like a system of special action handlers that will group them together

#

Like JumpHandler class, and this will group everything for jumping and validate + invoke it

mighty radish
#

most of the variables do have a different purpose, though similar, but theres definite some that might be duplicates, and even not used because i forget what they are for. im tryingto clean them up / consolidate them,

tardy cloak
#

You don't have any context in the editor though, so if you want to be able to configure jumping you would actually have to put it in the main class

#

Or serialize the Jump handler so they show, or create a custom editor

tardy cloak
#

Magic numbers are not bad, considering you probably have a single value here

tardy cloak
#

Also considering the size of the file it doesn't hurt to add this little bit of readability

tardy cloak
#

JumpHandler, CrouchHandler

mighty radish
#

@tardy cloak so about public variables. i could probably use serializedfield only many of them, but i also have many of them that need to be modified from other scripts. this might be because i am not using things like structs, classes, etc. and mostly i am just using variables to do things.

tardy cloak
mighty radish
#

so thats one reason why iv stuck with using public

tardy cloak
#

If there's some that need to be modified then you should probably use a separate method, or change them into properties. Properties allow you to add a bit of extra behaviour to check if the variable is correct but usually you'd make a method

#

For example, jump height. If you set this you want to validate the height is not negative or above a certain limit, so you make the field private and then add a method to set it, which validates it

#

Properties do the same thing technically, these are converted internally to private fields with a set method

#

But people don't expect properties to have complex setters as much as methods would do

mighty radish
#

its mainly because i am not 'familiar' or comfortable with using 'methods or properties to do such things. effecting variables for me seems easier. but its something i think about

mighty radish
#

but im sure i have some scripts that need to effect some of them, i just cant ID any at the moment.

tardy cloak
#

It would lead to bugs where stuff modifies data and you didn't expect it for example

tardy cloak
#

The compiler will list all the missing references

tardy cloak
#

If you configured VS correctly you can also list references on properties, methods and classes

#

You can't do that with fields

gaunt barn
mighty radish
#

well there is the right click ' find all references'. but with this specific script i think most if not all of them can be private. now that youve questioned it.

tardy cloak
#

It's also just making sure that it's not being modified without having to spend time looking for if there is an instance of this

#

With private you're sure only the code modifies it

gaunt barn
#

A lot of your variables seem to be providing the same functionality, but in different states, so maybe a state machine is also one thing that can make things more ordered.

#

Or rather, encapsulating data in states.

mighty radish
#

yeah i literally only know how to use variables thats why there is so much of them and i am not using anything like state machines, structs, etc.

#

i know so much needs to be worked on

#

i imagine an expert would achieve the same logic / functionality more efficiently in under 1,000 lines of code. maybe under 500