HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

My game manager

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
managergamestackoverflow

Problem

This is my game manager script and is there anyway i can improve this code.

```
using UnityEngine;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using UnityEngine.SceneManagement;

public class GameManager : Singleton
{

public Game game=new Game();

public PendingGamesRoot pendingGames = new PendingGamesRoot();

public List currentWord = new List();

public AudioClip wordFound;

public UserStatistics statistics;

protected virtual void Start ()
{
EventsManager.instance.CreateSolitareGameEvent += CreateSolitareGame;
EventsManager.instance.CreateGuruGameEvent += CreateMultiplayerGame;
EventsManager.instance.cardSelectedEvent += CheckIfWordFound;
EventsManager.instance.wordFound += WordFound;
StartCoroutine (GettingPendingGames());
}

protected virtual void OnDisable ()
{
if(EventsManager.instance!=null)
{
EventsManager.instance.CreateSolitareGameEvent -= CreateSolitareGame;
EventsManager.instance.CreateGuruGameEvent -= CreateMultiplayerGame;
EventsManager.instance.cardSelectedEvent -= CheckIfWordFound;
EventsManager.instance.wordFound -= WordFound;
}
}

IEnumerator GettingPendingGames()
{
while(true)
{
if(SceneManager.GetActiveScene().name!=Scene.menu)
return;
string url = Url.getPendingGames + FB.UserId;
WWW www = new WWW (url);
yield return www;
PendingGamesRoot temp = www.text.ToClass();
if(temp.ToJson()!=pendingGames.ToJson())
{
GetPendingGames ();
}
yield return new WaitForSeconds (10);
}
}

public void GetPendingGames()
{
string url = Url.getPendingGames + FB.UserId;
MyRequest.CreateRequest (url,GetPendingGamesCallback);
}

void GetPendingGamesCallback(WWW www)
{

Solution

I must say that I don't fully understand your manager, because I don't see the full implementation (EventManager, WBGuru, WWW). But I'll try to add some comments\improvements anyway:

  • I'm not familiar with your singleton implementation, but as I see, nothing stops me from creating my own GameManager. I think that if you want it to be a real Signleton, make it private.



  • What is the Game member? Is it the active\current game? Because I see you assign it per request, and the GameManger should support multiple games (I mean multiple games and not one game with multiple players). If it is not the active game, why do you need to save it as class member? Create it when requested.



  • All your members are public, so why should I use your game manager logic? I can just create one and assign pendingGames and game myself. I can even change the statistics and be the champ :-)! You must encapsulate them.



  • I see that sometimes you check for null in EventManager.instance and sometimes you don't. If instance is implemented as Sigleton accessor, you don't need to do a null checking, it will never be null (even if it is null, once you access it to check for null, you will create it..). If it's not a Singleton and can be null, check always and not just for the disable.



  • For public members\enums\const\statics, use a capital letter (convention)



  • In CheckIfWordFound you use First linq expression. Are you sure that you will always get a result? Otherwise you will get exception. Maybe it's better to use FirstOrDefauld and check for null.



  • In same function: almost always it's not a good idea to use an empty catch.



I must say again, I am not sure I see the full picture, so excuse me if some of my comments are not relevant.

Context

StackExchange Code Review Q#114954, answer score: 6

Revisions (0)

No revisions yet.