patterncsharpMinor
My game manager
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)
{
```
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 (
I must say again, I am not sure I see the full picture, so excuse me if some of my comments are not relevant.
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
Gamemember? Is it the active\current game? Because I see you assign it per request, and theGameMangershould 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
pendingGamesandgamemyself. I can even change the statistics and be the champ :-)! You must encapsulate them.
- I see that sometimes you check for
nullinEventManager.instanceand sometimes you don't. If instance is implemented as Sigleton accessor, you don't need to do anullchecking, it will never benull(even if it isnull, once you access it to check fornull, you will create it..). If it's not a Singleton and can benull, check always and not just for the disable.
- For public members\enums\const\statics, use a capital letter (convention)
- In
CheckIfWordFoundyou useFirstlinq expression. Are you sure that you will always get a result? Otherwise you will get exception. Maybe it's better to useFirstOrDefauldand check fornull.
- 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.