New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Proposal] Embrace dependency injection vs super class extension #25

Closed
SammyK opened this Issue May 1, 2014 · 4 comments

Comments

Projects
None yet
4 participants
@SammyK
Collaborator

SammyK commented May 1, 2014

I just need to get a blessing on this from you guys before we move forward with any major refactors.

The Problem

Single Responsibility Violations

Right now we have to extend the super class in order to add custom functionality. For example if I wanted to use my own persistent data storage handler, I'd need to extend the FacebookRedirectLoginHelper and overwrite the storeState() and loadState() methods.

There are a few problems with this type of customizability.

As it stands, FacebookRedirectLoginHelper is violating the single responsibility design principal. This class should only concern itself with helper methods that directly relate to gaining a FacebookSession object from a redirect. It should not have to know about how to store persistent data.

It's also causing other issues as pointed out in #8. :)

So it needs to be refactored. We need to pull out the persistent store methods into their own class that's concerned with nothing more than persistent data storage. This will also be helpful for future versions of the Graph API that might require we store persistent data for another class.

As pointed out in #4, a single responsibility violation also exists for the FacebookRequest class. It should only be concerned with what request it's making, not how it makes the requests.

Custom Named Classes Are A Bitch

Another issue with super class extension in this context is that all the existing nomenclature goes to hell and refactoring is a bitch. To be frank. :)

So you'd have some classes that are named MyFacebookSession and others with the default FacebookRequest name, and then if you need to customize the FacebookRequest object later, you have to go through all your code and rename every reference to the FacebookRequest to MyCustomFacebookRequest. Eeew.

Dependency Injection To The Rescue!

Both of these issues can be fixed with a refactor (and they will be soon!) but I need your blessing on dependency injection with an interface.

So FacebookRedirectLoginHelper will get a new static variable that is type hinted with the PersistentDataStoreInterface interface and a custom implementation can be injected with a setter. And FacebookRequest will get a new static variable that has is type hinted with the HttpClientInterface interface and a custom implementation can be injected with a setter.

If I can get your blessing on this (say yes!) then I'll start the refactor for both. But I have one more question on naming the interfaces. We have two main options.

  1. Suffix all interfaces with <NAME>Interface. So it'd be HttpClientInterface and PersistentDataStoreInterface.
  2. Follow the more human-readable, "able" suffix. So it'd be Httpable and Persistentable.

I've used both conventions and I'm kinda loving on option 2. :)

@ilves

This comment has been minimized.

ilves commented May 1, 2014

I like the dependency injection idea :)

@yguedidi

This comment has been minimized.

Collaborator

yguedidi commented May 1, 2014

👍 ! I'll also move exceptions and helpers to there own namespace (Facebook\Exception and Facebook\Helper)

@yguedidi

This comment has been minimized.

Collaborator

yguedidi commented May 1, 2014

About FacebookSession, it's too complex. I think it should be splitted in 2 classes :

  • FacebookSession with a fixed application, session infos, long lived, exchange token and validation functions.
  • FacebookSessionFactory with all other functions, that create FacebookSession instances for a specific application or for the default one.
@gfosco

This comment has been minimized.

Contributor

gfosco commented May 15, 2014

Going to close this.. If anything is still in question, please open a fresh issue for specific items. Thank you.

@gfosco gfosco closed this May 15, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment