search

ImLewel / ImLewel_FPS_Shooter

My Unity project
MIT License
1 stars 0 forks source link

Code review #1

Open tshemsedinov opened 1 year ago

tshemsedinov commented 1 year ago
  1. Hardcoded magic numbers: https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/bcce7011a6e883ac5a618112acebaa971ee92d69/Assets/TextMesh%20Pro/Examples%20%26%20Extras/Scripts/TextMeshSpawner.cs#L33 https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/bcce7011a6e883ac5a618112acebaa971ee92d69/Assets/TextMesh%20Pro/Examples%20%26%20Extras/Scripts/TextMeshSpawner.cs#L46 https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/414fdeabe632820805c2fee07e1463a1841a88bd/Assets/Scripts/GameScripts/Movement.cs#L134-L135 https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/bcce7011a6e883ac5a618112acebaa971ee92d69/Assets/TextMesh%20Pro/Examples%20%26%20Extras/Scripts/VertexShakeB.cs#L163 https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/bcce7011a6e883ac5a618112acebaa971ee92d69/Assets/TextMesh%20Pro/Examples%20%26%20Extras/Scripts/TextMeshProFloatingText.cs#L31 https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/414fdeabe632820805c2fee07e1463a1841a88bd/Assets/Scripts/GameScripts/Movement.cs#L75 https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/bcce7011a6e883ac5a618112acebaa971ee92d69/Assets/TextMesh%20Pro/Examples%20%26%20Extras/Scripts/TextMeshProFloatingText.cs#L186 and multiple other places
  2. Code duplication, you need to organize higher abstractions and extract two parts from duplicated blocks: invariant and variant to prevent copy/paste: https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/bcce7011a6e883ac5a618112acebaa971ee92d69/Assets/TextMesh%20Pro/Examples%20%26%20Extras/Scripts/TextMeshSpawner.cs#L28-L74
  3. Instead of https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/a50016bebeccb00e270c54712b49cbcbd2599c91/Assets/Scripts/MainMenuScripts/SettingsManager.cs#L14-L17 better use ternary: 
    QualitySettings.vSyncCount = QualitySettings.vSyncCount != 1 ? 1 : 0;
  4. This can be easily rewritten without switch/case: https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/414fdeabe632820805c2fee07e1463a1841a88bd/Assets/Scripts/GameScripts/Movement.cs#L132-L142 to reduce code duplication and remove hardvoced values
  5. Long and complex method is s subject to decompose: https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/414fdeabe632820805c2fee07e1463a1841a88bd/Assets/Scripts/GameScripts/Movement.cs#L180-L211 and here to:
  6. Nested if-statements can be converted to single one and you can use intermediate variables to remove calls from if-statemets to reduce its complexity: https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/ab469230b41abb6fc3f02b78bf078856657af283/Assets/Scripts/GameScripts/Inventory.cs#L39-L40
  7. Duplicate code (calls), use intermediate variables and ternary operator to rewrite: https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/ab469230b41abb6fc3f02b78bf078856657af283/Assets/Scripts/GameScripts/Inventory.cs#L53-L56
  8. Long and comples terrible carpet of code is a subject to decompose: https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/bcce7011a6e883ac5a618112acebaa971ee92d69/Assets/TextMesh%20Pro/Examples%20%26%20Extras/Scripts/VertexShakeB.cs#L52-L182
  9. We call this antipattern "pretty columns": https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/bcce7011a6e883ac5a618112acebaa971ee92d69/Assets/TextMesh%20Pro/Examples%20%26%20Extras/Scripts/VertexShakeB.cs#L126-L129
  10. Decompose: https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/bcce7011a6e883ac5a618112acebaa971ee92d69/Assets/TextMesh%20Pro/Examples%20%26%20Extras/Scripts/TextMeshProFloatingText.cs#L49-L95
  11. Decompose: https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/bcce7011a6e883ac5a618112acebaa971ee92d69/Assets/TextMesh%20Pro/Examples%20%26%20Extras/Scripts/TextMeshProFloatingText.cs#L113-L166
  12. Decompose: https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/bcce7011a6e883ac5a618112acebaa971ee92d69/Assets/TextMesh%20Pro/Examples%20%26%20Extras/Scripts/TextMeshProFloatingText.cs#L169-L221
  13. Totally terrible file: https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/bcce7011a6e883ac5a618112acebaa971ee92d69/Assets/TextMesh%20Pro/Examples%20%26%20Extras/Scripts/TMP_TextSelector_B.cs
  14. Commented code may appear only in debug or development stages but not at master branch
  15. Use ternary: https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/bcce7011a6e883ac5a618112acebaa971ee92d69/Assets/TextMesh%20Pro/Examples%20%26%20Extras/Scripts/CameraController.cs#L55-L58
  16. Long lines are not readambe, use intermediate variables to split a line into a few: https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/bcce7011a6e883ac5a618112acebaa971ee92d69/Assets/TextMesh%20Pro/Examples%20%26%20Extras/Scripts/CameraController.cs#L94 https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/bcce7011a6e883ac5a618112acebaa971ee92d69/Assets/TextMesh%20Pro/Examples%20%26%20Extras/Scripts/CameraController.cs#L104 https://github.com/ImLewel/ImLewel_FPS_Shooter/blob/bcce7011a6e883ac5a618112acebaa971ee92d69/Assets/TextMesh%20Pro/Examples%20%26%20Extras/Scripts/CameraController.cs#L114

Same problems you have over all other codebase

ImLewel commented 1 year ago
  1. Most of examples you have provided are assets code, by Unity if remember right, only in 3rd and 6th is my code, I'll change this values when finish certain tests.
  2. Also not mine code
  3. Took in consideration
  4. I agree, but this is recently added code so I didn't done refactoring during tests, as I said
  5. I want to find better approach to do necessarily checks, so it will be updated and refactored
  6. Got it
  7. Same
  8. Not mine code
  9. Same
  10. Same
  11. Same
  12. Same
  13. Same
  14. Thanks for advice
  15. Same as 13 and before 13
  16. Same