Facebook helper class

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
{
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(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(result.Te

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. 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.

StackExchange Code Review Q#114828, answer score: 3