2
\$\begingroup\$

Hello I'm currently developing a FPS game for my portfolio and I made a weapon shoot system, and I would like to receive feedback about my code to make more cleaner and efficient. Thanks.

using UnityEngine;

[RequireComponent(typeof(PlayerGunController))]

public class PlayerGunShootManager : MonoBehaviour
{

    private PlayerGunController _playerGunController;

    [SerializeField] private int _bulletDamage;

    [SerializeField] private int _gunMagazineSize;

    [SerializeField] private int _bulletsPerShoot;

    [SerializeField] private float _shootDelay;

    [SerializeField] private float _bulletSpreadRange;

    [SerializeField] private float maxDistanceTheBulletCanHit;

    [SerializeField] private LayerMask _bulletLayerMask;

    [SerializeField] private Transform bulletSpawmPosition;

    private float _gunReloadTime;

    private bool _playerCanShoot = true;

    private int _bulletsLeft;

    private Vector3 _randomBulletSpreadRangeValue = new Vector3(0f, 0f, 0f);

    private readonly RaycastHit[] rayHit = new RaycastHit[4];

    private float _defaultBulletSpreadRangeValue;

    private const string EnemyTag = "Enemy";

    public bool PlayerIsShooting { get; private set; }

    public bool PlayerIsReloading { get; private set; }

    private void Awake()
    {
        _playerGunController = GetComponent<PlayerGunController>();

        _defaultBulletSpreadRangeValue = _bulletSpreadRange;
    }
    
    private void Start()
    {
        _playerGunController.GetPlayerController().PlayerInputsManager.OnPlayerIsPressingShootButton += PlayerIsPressingShootButton;

        _playerGunController.GetPlayerController().PlayerInputsManager.OnPlayerStoppedPressingShootButton += PlayerHasStoppedShooting;

        _playerGunController.GetPlayerController().PlayerInputsManager.OnPlayerPressedReloadButton += ReloadGun;

        _playerGunController.GetPlayerController().PlayerInputsManager.OnPlayerIsPressingAimButton += ChangeBulletSpreadValueToMorePrecise;

        _playerGunController.GetPlayerController().PlayerInputsManager.OnPlayerStoppedPressingAimButtom += ChangeBulletSpreadRangeToDefaultValue;

        _bulletsLeft = _gunMagazineSize;
        
        _gunReloadTime = _playerGunController.PlayerGunAnimationManager.GetGunReloadAnimationTime();
    }

    private void Update()
    {
        if (CheckIfPlayerCanShoot() && PlayerIsShooting)
        {
            Shoot();
        }
    }
    private bool CheckIfPlayerCanShoot()
    {
        return _playerCanShoot && !PlayerIsReloading && _bulletsLeft > 0;
    }

    private void Shoot()
    {
        _playerCanShoot = false;

        _bulletsLeft -= _bulletsPerShoot;

        Vector3 directionBulletRaycastShouldGo = bulletSpawmPosition.forward + GetRandomBulletSpreadValue();

        if (Physics.RaycastNonAlloc(transform.position, directionBulletRaycastShouldGo, rayHit, maxDistanceTheBulletCanHit, _bulletLayerMask) > 0)
        {
            for (int i = 0; i < rayHit.Length; i++)
            {
                if (rayHit[i].collider != null && rayHit[i].collider.gameObject.CompareTag(EnemyTag))
                {
                    Damageable damageable = rayHit[i].collider.gameObject.GetComponent<Damageable>();

                    damageable.TakeDamage(_bulletDamage);
                    break;
                }
            }
        }

        if (_bulletsLeft > 0)
        {
            Invoke(nameof(AllowPlayerShootAgain), _shootDelay);
            return;
        }

        ReloadGun();
    }

    private void ChangeBulletSpreadRangeToDefaultValue()
    {
        _bulletSpreadRange = _defaultBulletSpreadRangeValue;
    }

    private void ChangeBulletSpreadValueToMorePrecise()
    {
        _bulletSpreadRange *= 0.5f;
    }

    private Vector3 GetRandomBulletSpreadValue()
    {
        _randomBulletSpreadRangeValue.x = Random.Range(-_bulletSpreadRange, _bulletSpreadRange);
        _randomBulletSpreadRangeValue.y = Random.Range(-_bulletSpreadRange, _bulletSpreadRange);

        return _randomBulletSpreadRangeValue;
    }

    private void AllowPlayerShootAgain()
    {
        _playerCanShoot = true;
    }

    private void PlayerHasStoppedShooting()
    {
        PlayerIsShooting = false;
        PlayerGlobalGunManager.SetPlayerIsShootingToFalse();
    }

    private void PlayerIsPressingShootButton()
    {
        PlayerIsShooting = true;

        PlayerGlobalGunManager.SetPlayerIsShootingToTrue();
    }

    private void ReloadGun()
    {
        if (!PlayerIsReloading && _bulletsLeft < _gunMagazineSize)
        {
            PlayerIsReloading = true;

            PlayerGlobalGunManager.SetPlayerIsReloadingToTrue();

            Invoke(nameof(ReloadFinished), _gunReloadTime);
        }
    }

    private void ReloadFinished()
    {
        _bulletsLeft = _gunMagazineSize;

        PlayerIsReloading = false;

        PlayerGlobalGunManager.SetPlayerIsReloadingToFalse();

        AllowPlayerShootAgain();
    }
}
\$\endgroup\$
0

1 Answer 1

2
\$\begingroup\$
_playerGunController.GetPlayerController().PlayerInputsManager.OnPlayerIsPressingShootButton += PlayerIsPressingShootButton;

_playerGunController.GetPlayerController().PlayerInputsManager.OnPlayerStoppedPressingShootButton += PlayerHasStoppedShooting;

_playerGunController.GetPlayerController().PlayerInputsManager.OnPlayerPressedReloadButton += ReloadGun;

_playerGunController.GetPlayerController().PlayerInputsManager.OnPlayerIsPressingAimButton += ChangeBulletSpreadValueToMorePrecise;

_playerGunController.GetPlayerController().PlayerInputsManager.OnPlayerStoppedPressingAimButtom += ChangeBulletSpreadRangeToDefaultValue;

That's a lot of copying. You can put _playerGunController.GetPlayerController().PlayerInputsManager into a variable and use it instead of retrieving it again every time.

private bool CheckIfPlayerCanShoot()
{
    return _playerCanShoot && !PlayerIsReloading && _bulletsLeft > 0;
}

If this method decides whether the player can shoot or not, then what is _playerCanShoot. It turns out that sometimes _playerCanShoot == true (which suggests to the reader that the player can indeed shoot) but CheckIfPlayerCanShoot() will return false. Who should we trust?

If you can shoot if _bulletsLeft > 0, nothing stops you from shooting with one bullet in the magazine even if your _bulletsPerShoot is higher than one which makes your _bulletsLeft negative.

if (Physics.RaycastNonAlloc(transform.position, directionBulletRaycastShouldGo, rayHit, maxDistanceTheBulletCanHit, _bulletLayerMask) > 0)
{
    for (int i = 0; i < rayHit.Length; i++)

rayHit.Length is always 4 since you made it that way. What you should check for is the result of Physics.RaycastNonAlloc(...) that you just got.

if (_bulletsLeft > 0)
    Invoke(nameof(AllowPlayerShootAgain), _shootDelay);
    return;
}

ReloadGun();

Again this doesn't take _bulletsPerShoot into account.

private void PlayerHasStoppedShooting()
{
    PlayerIsShooting = false;
    PlayerGlobalGunManager.SetPlayerIsShootingToFalse();
}

private void PlayerIsPressingShootButton()
{
    PlayerIsShooting = true;

    PlayerGlobalGunManager.SetPlayerIsShootingToTrue();
}

Those functions do the exact opposite things but their names don't reflect it.


Overall pretty well written, keep it up!

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Hey @QuasiStellar, thanks for the tips. I did the changes that you suggested and helped me to clean another codes with the same errors as well. I fixed the repeating and now I'm checking if _bulletsLeft >= _bulletsPerShoot. I changed the name of PlayerIsShooting to PlayerIsPressingShootButton because that is what the variable value is. I did some other changes too but I can't post the full code here because have a limited text, but thank you for the tips. \$\endgroup\$
    – Thomas
    Oct 24, 2022 at 13:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.