Facebook helper class

Question

This is my initial Facebook helper class. Is this code efficient or is there a way to improve this code?

using UnityEngine;
using System.Collections;
using Newtonsoft.Json;
using Facebook.MiniJSON;
using System.Linq;

public class FacebookManager : SingletonEternal<FacebookManager> 
{
    public string accessToken;
    public FacebookFriends fbFriendsResult;
    public MyFacebookInfo myInfo = new MyFacebookInfo();
    public Texture2D pic;

    IEnumerator Start () 
    {
        if (!FB.IsInitialized)
            FB.Init (null);

        while (!FB.IsInitialized)
            yield return null;

        #if UNITY_EDITOR
        FacebookManager.instance.Login();
        #endif

        if(FB.IsLoggedIn)
        {
            GetMyInformation();
            GetMyFriendsList();
        }
    }

    void SetMyPicture(WWW www)
    {
        pic = www.texture;
    }

    public void Login()
    {
        FB.Login ("user_friends, email, public_profile",LoginCallback);
    }

    void LoginCallback(FBResult result)
    {
        if(result.Error!=null)
        {

        }
        else
        {
            GetMyInformation();
            GetMyFriendsList();
        }
    }

    public void Logout()
    {
        FB.Logout ();
    }

    void GetMyInformation()
    {
        FB.API ("me?fields=id,name,picture",Facebook.HttpMethod.GET,GotMyInformationCallback);
    }

    void GotMyInformationCallback(FBResult result)
    {
        if(result.Error!=null)
        {

        }
        else
        {
            myInfo = JsonConvert.DeserializeObject<MyFacebookInfo>(result.Text);
            MyRequest.CreateRequest(myInfo.picture.data.url,SetMyPicture);
        }
    }

    void GetMyFriendsList()
    {
        FB.API ("me/friends?fields=name,picture",Facebook.HttpMethod.GET,GotMyFriendsListCallback);
    }

    void GotMyFriendsListCallback(FBResult result)
    {
        if(result.Error!=null)
        {

        }
        else
        {
            fbFriendsResult = JsonConvert.DeserializeObject<FacebookFriends>(result.Text);
            fbFriendsResult.data = fbFriendsResult.data.OrderBy(a=>a.name).ToList();
        }
    }

    public void Invite()
    {
        FB.AppRequest ("Invite friends!");
    }
}

Answer 1

Looks great. I have some comments..

1. Why MyFacbookInfo myInfo initalized in the start? you assign it in GetMyInforamtionCallback anyway.
2. member accessToken is never used.
3. In Start method, you do a while to wait to initalization to complete. I don't say its bad, but maybe you can achieve the same result in other technique. I don't familiar with the FaceBook API but maybe with some callback, or even with timer to avoid waste of CPU.
4. I'm very recommend you to use async/await in you login and get method to avoid blocking clients UI thread.
5. Is it on purpose that all your method return void? Why not to return the result?
6. Its prefer to use properties and not public members.

Answer 2

I'll refer you to this excellent blog post on Coding Horror in regard to the name of your class. XyzManager in general is never going to be a good name for a class.

You should sort your usings alphabetically and get rid of any that aren't used. Visual Studio (or resharper) can both do that automatically for you (using 'organise usings' or 'clean up code' respectively).

What is your base class? SingletonEternal<FacebookManager>. I could argue that singletons are always 'eternal' - at least for the lifetime of the application... so the name is a bit off too.

As already mentioned - you shouldn't normally use public fields. Properties are better for encapsulation as you can control who sets the value as well as who can read it. At the moment you're letting any consuming code update the values. Auto properties aren't even much more code:

public string AccessCode { get; set;}

This might sound a bit mean, but your code looks ugly to me... Let me explain.

Which is easier to read?

thisfirstone

or

the second one

Hopefully you'd agree that the second one is easier to read, the same is true for your code. Separate parameters with a space, do the same for operators:

FB.API ("me?fields=id,name,picture",Facebook.HttpMethod.GET,GotMyInformationCallback);

Becomes:

FB.API("me?fields=id,name,picture", Facebook.HttpMethod.GET, GotMyInformationCallback);

and result.Error!=null should be result.Error != null. The same applies here a=>a.name change it to a => a.name. It would be even nicer if you changed a to something meaningful.

I know it all seems really trivial but code will be read far more often than it's written/edited so you should be optimizing for read time not saving the odd space characters when you're typing it.

Answer 3

Instead of leaving the if branch empty like this:

if(result.Error!=null)
{

}
else
{
    myInfo = JsonConvert.DeserializeObject<MyFacebookInfo>(result.Text);
    MyRequest.CreateRequest(myInfo.picture.data.url,SetMyPicture);
}

It would be better to write like this:

if (result.Error == null)
{
    myInfo = JsonConvert.DeserializeObject<MyFacebookInfo>(result.Text);
    MyRequest.CreateRequest(myInfo.picture.data.url,SetMyPicture);
}