Clean human readable code ready for github push?

[crimson flame]

anyways, does this code look "clean" and human readable? I'm going to make this open source and I need the code to be ready, this format etc that is used in this script is used in all my scripts so that's why I'm showing specifically this one, so if I made a mistake with like, naming conventions, or readability, I can use that as help to fix the other classes and scripts

https://gdl.space/nonewunoki.cs

I think that's right?

[calm tangle]

yep, gimme a sec i’ll hop on my laptop

[crimson flame]

:)

[calm tangle]

Alright heres some initial impressions:

1. Do all those variables need to be public?
2. In the UpgradePlayer method, why dont you use an enum as the parameter instead of a string?

[crimson flame]

I tried using Enums but they don't apear in unity.event, meaning I can't give the player an upgrade based off of unity events

and the ones that are public need to be accessed outside of the script by things such as enemies, doors, locks, maps, traps, etc

I know you can subscribe to another class but I have no idea and haven't found any kind of documentation on how to do that, just heard it thrown around a few times

[bronze cargo]

I think you could start up by breaking classes down? This class has various responsibility

[crimson flame]

yeah but it controls the player, all aspects of it is still controlling the player so that's why I kept it all in one class.

I could seperate something like upgrades though but most of the other things I think fit into the class

[bronze cargo]

Game pausing, Health management, Weapon management, Cheat, etc.. IMO they should be separated

[crimson flame]

oh?

not trying to be dismisive or anything but I thought that Health management and Weapon management would still be part of a players controller?

also pausing already has a seperate script I just forgot to update the code sense I made that new script

the cheats are just for debugging but I do think it should be seperate yeah :) thanks for the advice

[bronze cargo]

My impression of term 'controller' is something movement-related, maybe including camera.

Weapons for example you might want different damage mechanism or cooldown mechanism per weapon. Separating class will help you modularizing those.

For health management, separating class lets you reuse same logic to non-players, monsters for example.

[crimson flame]

hmm, okay, I'll try seperating them them

also I was thinking of controller more like something that controls the players data :)

[crimson flame]

health is handled by a seperate class which works for both players and enimies, but both eniemies brain and players controler has a health field, which gets modified from the "HealthHandler"

[bronze cargo]

Clean-up wise I see some inconsistent SerializeField and capitalization. And the switch-case has some common code you might want to combine..

[calm tangle]

I really mostly have minor things left:

1. Line 116 HealthHandler healthbar = gameObject.GetComponent<HealthHandler>(); you should cache this. Same with line 119
2. Inside the shoot method, you can avoid the bullet get component by instantiating the bullet component instead of the game object. It will still spawn the bullet gameobject but returns the component

[SerializeField] private Bullet bullet;
// method
CanShoot = false;
Bullet I = Instantiate(bullet, spawnLocation.position, spawnLocation.rotation);
I.damage = PlayerStats.ATK;
Destroy(I.gameObject, 1);

[crimson flame]

SerilalizeField is so I can edit the values in the inspector but their unneeded to be edited by outside scritps and I'm fixing the capitilization now :)

[crimson flame]

Line 116 HealthHandler healthbar = gameObject.GetComponent<HealthHandler>(); you should cache this. Same with line 119
wdym?

I've never heard of caching something before

[calm tangle]

Same thing you are already doing with Rigidbody

[crimson flame]

oh keeping a referance of it?

like where I don't re-asign it

[calm tangle]

yep