CR by dekel31 · Pull Request #1 · dekel31/Jellybutton_Skyroads_Nadav_Etinzon

dekel31 · 2021-09-11T01:39:39Z

No description provided.

dekel31 · 2021-09-11T01:40:58Z

@@ -0,0 +1,11 @@
+using UnityEngine;
+
+[CreateAssetMenu(menuName = "Gameplay/Road Params", fileName = "Road Params")]


Don't know if it's a good idea to call file names with spaces

dekel31 · 2021-09-11T01:41:32Z

+
+public class GameplayObjectPool : IObjectPool
+{
+    #region Constants


remove regions that are empty, no code, no need for a region and empty lines

dekel31 · 2021-09-11T01:48:11Z

+using System.Collections.Generic;
+using UnityEngine;
+
+public class GameplayObjectPool : IObjectPool


This class seems to have a "manager" quality - it looks like it's generic, but in fact it deals with very specific objects, each in it's own individual name (look at the 2nd comment here as a good explanation why it's bad: https://softwareengineering.stackexchange.com/questions/129537/can-manager-classes-be-a-sign-of-bad-architecture)
If it creates objects, it should be a factory. if it's a factory, it should create only one type of object.

dekel31 · 2021-09-11T01:59:09Z

+using System.Collections.Generic;
+using UnityEngine;
+
+public abstract class ObstacleBaseFactory


this class seems redundant. you inherit it once, and you don't use the abstraction. you should delete it

dekel31 · 2021-09-11T01:59:33Z

+using System.Collections.Generic;
+using UnityEngine;
+
+public abstract class RoadBaseFactory


this class seems redundant. you inherit it once, and you don't use the abstraction. you should delete it

dekel31 · 2021-09-11T02:14:40Z

+    Obstacle
+}
+
+public interface IPooledObject


PooledObject means an object that an get the gameObject, in the end. it is too generic, any unity object can have a gameObject

dekel31 · 2021-09-11T02:14:50Z

+                } 
+                else
+                {
+                    var sinmpleRoad = (SimpleRoad)_simpleRoadFactory.Create();


dekel31 · 2021-09-11T02:19:54Z

+using System.Collections.Generic;
+using UnityEngine;
+
+public enum PooledObjectType


this type describes every object in the game, too generic. follow the main comment at the end of this to remove it completely too

dekel31 · 2021-09-11T02:21:03Z

+    public override IObstacle Create()
+    {
+        var asteroid = (GameObject)Object.Instantiate(_obstacleParams.AsteroidPrefab);
+        return asteroid.GetComponent<AsteroidObstacle>();


GetComponent doesn't necessarily get the object that you just created, see https://docs.unity3d.com/ScriptReference/GameObject.GetComponent.html (it might get you a different one that you created earlier)

dekel31 · 2021-09-11T02:29:08Z

+
+    #region Methods
+
+    public void AddObjectToPool(IPooledObject obj)


that is actually removing an object from the game, right?

dekel31 · 2021-09-11T02:35:23Z

+        var newObs = RandomizeNewObstacle();
+        if (newObs != null)
+        {
+            var pos = RandomPositionOnRoad();
+            GameplayElements.Instance.AddObstacleToRoad(newObs, _lastRoadIndex, pos);
+        }


this is a duplicate of lines 32-37. simply, you can send the parameters of AddObstacleToRoad to the RandomizeNewObstacle method, if it does create an obstacle it will use them, won't otherwise

dekel31 · 2021-09-11T02:43:17Z

+
+    public static void Initialize()
+    {
+        _awaitService = CreateAwaitService();


seems like a lot of these can be NativeSingleton s

dekel31 · 2021-09-11T02:43:58Z

I haven't looked at everything, but this is what I found so far:

dekel31 · 2021-09-11T02:48:30Z

first I'll begin with the things I don't like, but are not wrong. I'll state my reason for my disliking them.
You have an interface for everything, even ones that you use only once. imho, the less code, the better. shouldn't make a mountain out of a molehill. Some programmers create an interface for each class, in this day and age, tests are way easier to write than before, DI happens automatically, therefore this practice is dated.
Region - god I hate region. It creates so many empty lines and redundant lines. While you have an enormous 1k line class, fine, but for 50 line? you added 8-12 for regions.
That's my dislikes. onto things I think you should change, in the next comment

dekel31 · 2021-09-11T02:51:18Z

I think you don't need the IPooledObject interface. correct me if I'm wrong: for example, in AsteroidFactory, when instantiating the object, you use getComponent in order to get the "underlying" object of the GameObject that is the asteroid (BaseObstacle) and return it. but you never use the BaseObstacle itself (it's properties or whatnot), you just end up using GetGameObject (I'm referring to GameplayObjectPool: line 41 and using it in line 63. right after you get it in line 63 you immediately get the game object, so what is it for?). Happens in ObstacleController as well, you don't really use the type you created, so you can just refer to all game objects as themselves, not as the classes you created for them

dekel31 · 2021-09-11T03:06:10Z

Lastly, regarding GameplayObjectPool. this class is bound to get out of hand very fast with every new type of game component. As you can see, your switch case options are actually copies of one another:

case PooledObjectType.Road:
                _inActiveRoadsOP.Enqueue(obj.GetGameObject().GetComponent<BaseRoad>());
                break;

            case PooledObjectType.Obstacle:
                _inActiveObstaclesOP.Enqueue(obj.GetGameObject().GetComponent<BaseObstacle>());
                break;

instead, you can create a dictionary that it's key will be the object tag (see next comment) and the value will be the queue, something like this:

private tagsToGameObjects Dictionary<string, Queue<GameObject>> = new Dictionary<string, GameObject>();
tagsToGameObjects.Add("asteroid", new Queue<GameObject>());
tagsToGameObjects.Add("road", new Queue<GameObject>());
...
...
//instead of the switch case you do this:
public void AddObjectToPool(GameObject obj) {
tagsToGameObjects[gameObject.tag].Enqueue(obj));
...

dekel31 · 2021-09-11T03:07:49Z

I found something inside gameObject that can help differentiate between different types of them, instead of converting them into your own objects and then get the game object again. it's called tag https://docs.unity3d.com/ScriptReference/GameObject-tag.html

nadavEtin added 2 commits September 10, 2021 14:19

backup

369c898

end of day 3 working on bug with the obstacles

e449c36

dekel31 commented Sep 11, 2021

View reviewed changes

		@@ -0,0 +1,11 @@
		using UnityEngine;

		[CreateAssetMenu(menuName = "Gameplay/Road Params", fileName = "Road Params")]


		#region Methods

		public void AddObjectToPool(IPooledObject obj)

Conversation

dekel31 commented Sep 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dekel31 Sep 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dekel31 commented Sep 11, 2021

Uh oh!

dekel31 commented Sep 11, 2021

Uh oh!

dekel31 commented Sep 11, 2021

Uh oh!

dekel31 commented Sep 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dekel31 commented Sep 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dekel31 Sep 11, 2021 •

edited

Loading

dekel31 commented Sep 11, 2021 •

edited

Loading