Design
Try to put as little code as possible in your Start()
, Awake()
etc. methods. Have those methods call other methods. The reasoning is very similar to the reason behind avoiding code in the code-behind of WinForms or WPF applications. It seems like you could initialize those variables in the GetFriendRequests
method.
In your if statements, it is common to put the most-expected outcome first, and put the edge case in the else. In your case, a success case is more accepted than a failure, so put that first.
So
if (!String.IsNullOrEmpty(result.Error)) {
// Failed Case
lastResponse = "Get Friend Request Callback: Error Response:\n" + result.Error;
} else if (!String.IsNullOrEmpty(result.Text)) {
// Success Case
lastResponse = "Get Friend Request Callback: Success Response:\n" + result.Text;
JsonData datas = JsonMapper.ToObject(result.Text);
Debug.Log(datas["data"]);
for(int i = 0; i < datas["data"].Count; i++) {
requestData.Add(i, JsonMapper.ToObject((string)datas["data"][i]));
requestLoadStatus.Add(i, false);
requestTexture.Add(i, new Texture2D(50, 50));
StartCoroutine(LoadUserProfilePic(i));
}
}
should be:
if (!String.IsNullOrEmpty(result.Text)) {
// Success Case
lastResponse = "Get Friend Request Callback: Success Response:\n" + result.Text;
JsonData datas = JsonMapper.ToObject(result.Text);
Debug.Log(datas["data"]);
for(int i = 0; i < datas["data"].Count; i++) {
requestData.Add(i, JsonMapper.ToObject((string)datas["data"][i]));
requestLoadStatus.Add(i, false);
requestTexture.Add(i, new Texture2D(50, 50));
StartCoroutine(LoadUserProfilePic(i));
}
} else if (String.IsNullOrEmpty(result.Error)) {
// Failed Case
lastResponse = "Get Friend Request Callback: Error Response:\n" + result.Error;
}
Spacing
You're cramping your code together and making it much harder to read. Spread it out a bit and things will look a lot less cluttered and complex.
e.g
protected void GetFriendRequests() {
if(FB.IsInitialized) {
FB.API("/me/apprequests", Facebook.HttpMethod.GET, GetFriendRequestsCallback);
} else {
InitFacebook();
}
}
Is easier to read as:
protected void GetFriendRequests()
{
if(FB.IsInitialized)
{
FB.API("/me/apprequests", Facebook.HttpMethod.GET, GetFriendRequestsCallback);
}
else
{
InitFacebook();
}
}
I know the condensed form is how MonoDevelop decides to do bracing by default, which AFAIK is the Java style, but I prefer the expanded style for ease of reading, and it is the more common of the C# styles.
Secondly, it often helps to add breathing room around clauses in your code that use curly braces.
e.g.
// Success Case
lastResponse = "Get Friend Request Callback: Success Response:\n" + result.Text;
JsonData datas = JsonMapper.ToObject(result.Text);
Debug.Log(datas["data"]);
for(int i = 0; i < datas["data"].Count; i++)
{
requestData.Add(i, JsonMapper.ToObject((string)datas["data"][i]));
requestLoadStatus.Add(i, false);
requestTexture.Add(i, new Texture2D(50, 50));
StartCoroutine(LoadUserProfilePic(i));
}
Looks easier to read as:
// Success Case
lastResponse = "Get Friend Request Callback: Success Response:\n" + result.Text;
JsonData datas = JsonMapper.ToObject(result.Text);
Debug.Log(datas["data"]);
for(int i = 0; i < datas["data"].Count; i++)
{
requestData.Add(i, JsonMapper.ToObject((string)datas["data"][i]));
requestLoadStatus.Add(i, false);
requestTexture.Add(i, new Texture2D(50, 50));
StartCoroutine(LoadUserProfilePic(i));
}
Magic Variables
You're using magic variables in your code. Refactor these out as constants or static readonly variables.
e.g.
protected void GetFriendRequests()
{
if(FB.IsInitialized)
{
FB.API("/me/apprequests", Facebook.HttpMethod.GET, GetFriendRequestsCallback);
}
else
{
InitFacebook();
}
}
Should be:
private const string FacebookApiURL = "/me/apprequests";
protected void GetFriendRequests()
{
if(FB.IsInitialized)
{
FB.API(FacebookApiURL, Facebook.HttpMethod.GET, GetFriendRequestsCallback);
}
else
{
InitFacebook();
}
}
The reason you should do this is that if re-read the code it is more obvious what "/me/apprequests"
means, which is invaluable when performing maintenance on year-old code.
Testing
Lastly, I am concerned by the heavy quantity of Debug.Log
calls. I hope you're not testing your methods by writing status updates to the log, and are instead doing this so you can remain informed. If you are testing this way, stop right now. It's intensely inefficient, and Unity have released a free set of libraries for unit and integration tests that work very well with the engine.