Getting Facebook app requests

Question

I have a JSON response from the Facebook API, which contains the app requests of certain Facebook apps via FB.API('/me/apprequests') (using Graph API 2.0, Facebook SDK for Unity 6.0).

There are 3 things I want to collect from the requests:

1. The response data (a JSON string)
2. A texture (in Texture2D class)
3. Status of loading texture (in bool)

Here is how I achieve it:

private Dictionary<int, JsonData> requestData;
private Dictionary<int, bool> requestLoadStatus;
private Dictionary<int, Texture2D> requestTexture;

void Start() {
    loaded = false;
    requests = new ArrayList();
    requestData = new Dictionary<int, JsonData>();
    requestLoadStatus = new Dictionary<int, bool>();
    requestTexture = new Dictionary<int, Texture2D>();
    GetFriendRequests();
}

protected void GetFriendRequests() {
    if(FB.IsInitialized) {
        FB.API("/me/apprequests", Facebook.HttpMethod.GET, GetFriendRequestsCallback);
    } else {
        InitFacebook();
    }
}

private void GetFriendRequestsCallback(FBResult result) {
    if(result != null) {
        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));
            }
        }
        Debug.Log(lastResponse);
    } else {
        Debug.Log("Get Friend Request Callback: Response is empty");
    }
}

Where InitFacebook() is simply calling FB.Init(). Note that I used LitJson as a JSON parser.

How can I simplify this code? Using 3 Dictionarys seems to be clumsy. Will there be a benefit if I use a custom class to wrap them?

Answer 1

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.