4
\$\begingroup\$

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!");
    }
}
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Is this code finished? \$\endgroup\$
    – dreza
    Dec 23 '15 at 9:04
  • \$\begingroup\$ Posting on feed and inviting friends features will be added in the future but as far as the current scope goes this is a finished code, yes. \$\endgroup\$ Dec 23 '15 at 10:21
2
\$\begingroup\$

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.
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the insight, will make to do those all those changes. The one thing i have been wondering always was why use properties instead of public variables!? I've not understood the usage of properties so far so forgive my ignorance. \$\endgroup\$ Dec 23 '15 at 10:27
  • \$\begingroup\$ @KamalKannan Its not answer for comment here, but in general, its because then you control your internal data and behavior. Maybe here you just want to expose to everybody your internal data but maybe later you will decide to expose friends according to some condition. So you need a private member to hold all friends and a public getter to do some logic before you return friends to user. Please make a google search about that and you will find more detailed answer. \$\endgroup\$ Dec 23 '15 at 11:14
3
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks. Except for the singleton part you are true about everything else, i will certainly do these changes and follow them in the future. In unity a game is divided into several scenes and when moving from one scene to another all objects are destroyed, singleton or not. So i have two types of singletons one which is just needed for the level and the one which will be needed throughout the game irrespective of the level, that's why the name, ergo I have both Singleton and SingletonEternal in my project. \$\endgroup\$ Dec 24 '15 at 5:19
  • \$\begingroup\$ @KamalKannan that makes sense but I would have called them LevelSingleton and GameSingleton \$\endgroup\$
    – RobH
    Dec 24 '15 at 8:11
2
\$\begingroup\$

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);
}
\$\endgroup\$
1
  • \$\begingroup\$ Thanks. I left the if block empty to add error handling later. \$\endgroup\$ Dec 23 '15 at 10:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.