My game manager

Question

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<GameManager> 
{
 public Game game=new Game();
 public PendingGamesRoot pendingGames = new PendingGamesRoot();
 public List<string> currentWord = new List<string>();
 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<PendingGamesRoot>();
 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)
 {
 pendingGames = www.text.ToClass<PendingGamesRoot> ();
 if ((pendingGames.data.yourturns.Count - 1)!= statistics.activegames)
 statistics.UpdateNoOfActiveGames ((pendingGames.data.yourturns.Count));
 if (SceneManager.GetActiveScene().name==Scene.menu)
 {
 WBGuru.instance.CheckForBotGames();
 NotificationsGUI.instance.ConstructGUIForThePendingGames ();
 }
 }
 void CreateSolitareGame()
 {
 MyRequest.CreateRequest (Url.createSolitareGame,CreateSolitareGameCallback);
 }
 public void CreateSolitareGameCallback(WWW request)
 {
 game = request.text.ToClass<Game> ();
 game.mode = GameMode.solitare;
 SceneManagerExtension.instance.LoadScene (Scene.game);
 }
 public void GetStats()
 {
 string link = Url.getMember;
 link = link.Replace ("{name}",FB.UserId);
 MyRequest.CreateRequest (link,GetStatsCallback);
 }
 void GetStatsCallback(WWW www)
 {
 statistics = www.text.TrimLooseEnds().ToClass<UserStatisticsRoot>().member;
 }
 void CreateMultiplayerGame(string opponent)
 {
 string link = Url.createMultiplayerGame.Replace ("{player1}",FB.UserId);
 link = link.Replace ("{player2}",opponent);
 MyRequest.CreateRequest (link,CreateMultiplayerGameCallback);
 }
 void CreateMultiplayerGameCallback(WWW ww)
 {
 game.mode = GameMode.multiplayer;
 game.multiplayerGame = ww.text.ToClass<MultiplayerGameRoot>();
 game.Init ();
 game.GetRoundInfos ();
 SceneManagerExtension.instance.LoadScene (Scene.game);
 } 
 void CheckIfWordFound(Card card)
 {
 string wordFormed = string.Join ("",currentWord.ToArray());
 try
 {
 Word word = new Word();
 if(game.mode==GameMode.solitare)
 {
 word = game.data.worddefs.Where(a=>a.word==wordFormed.ToLower()).First();
 }
 else if(game.mode==GameMode.multiplayer)
 {
 word = game.roundWords.data.worddef.Where(a=>a.word==wordFormed.ToLower()).First();
 }
 if(!game.wordsFound.Contains(word))
 {
 EventsManager.instance.wordFound(word);
 game.wordsFound.Add(word);
 }
 }
 catch{}
 }
 void WordFound(Word word)
 {
 GameObject obj = new GameObject ("wordfound");
 AudioSource source = obj.AddComponent<AudioSource>();
 source.clip = wordFound;
 source.Play ();
 Destroy (obj,wordFound.length);
 for(int i=0;i<currentWord.Count;i++)
 {
 currentWord[i] = "";
 }
 if(word.score>statistics.bestWordScore)
 {
 statistics.UpdateWord(word);
 }
 Utility.DisableButtons ();
 GameGUI.instance.ClearsCards (2);
 }
}

I created a separate class to handle all the WWW request. It is attached below.

using UnityEngine;
using System.Collections;
public class MyRequest : MonoBehaviour
{
 WWW request;
 void Start()
 {
 DontDestroyOnLoad (gameObject);
 }
 IEnumerator InitiateWebRequest(string url,MyRequestCallback callback=null)
 {
 request = new WWW (url);
 yield return request;
 if(callback!=null)
 callback (request);
 LoadingScreen.instance.StopLoading ();
 Destroy (gameObject);
 }
 public static void CreateRequest(string url,MyRequestCallback callback=null)
 {
 LoadingScreen.instance.StartLoading ();
 GameObject obj = new GameObject("WebRequest");
 MyRequest script = obj.AddComponent<MyRequest>();
 script.StartCoroutine(script.InitiateWebRequest (url,callback));
 }
}
public delegate void MyRequestCallback(WWW request);

Link to whole project if you want to see understand the connection to other script dependencies here.

https://github.com/kannan21jan/wordsgame

• anon: Please explain in more detail what this game manager does, rather than saying "it's my game manager". What game? What does it manage? How does it manage it?

Answer 1

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:

1. 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.
2. 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.
3. 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.
4. 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.
5. For public members\enums\const\statics, use a capital letter (convention)
6. 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.
7. 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.

• Kamal Kannan: I have attached my link to the remaining scripts. If you want to look further into this.