Skip to content

Cleanup duplicate logic and refactor installations.js into InstallationsRouter. #357

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

Merged
merged 1 commit into from
Feb 11, 2016

Conversation

nlutsenko
Copy link
Contributor

Depends on #334.

  • Reuse duplicate logic instead of writing it twice
  • InstallationsRouter is a subclass of ClassesRouter and calls into super class methods

@drew-gross, what do you think about this approach over the custom function-based classes implementation that you have?
It's super clear what calls into what if we follow this approach.

Side-question - do we want/need to handle the same logic for body.keys/relations (redirectClassNameForKey) as for all other Classes?
Asking, 'for a friend', because we don't seem to do it right now.

@facebook-github-bot
Copy link

@nlutsenko updated the pull request.

@drew-gross
Copy link
Contributor

This also works and is still pretty readable, although with no member variables the advantage having any type of classes, functional or not, seems pretty minimal. Calling super.handleFind doesn't seem any better than exporting a handleFind function from ClassRouter and calling 'ClassRouter.handleFind. I guess if you want to insert another subclass betweenClassRouterandInstallationRouter` it could help, but an equivalent can also be arranged using standalone functions. As always I don't want to impose my personal style so even if this isn't the exact way I would write it, feel free to merge regardless :)

@nlutsenko nlutsenko force-pushed the nlutsenko.router.installations branch from 453dfbc to 6a12744 Compare February 11, 2016 21:50
@facebook-github-bot
Copy link

@nlutsenko updated the pull request.

nlutsenko added a commit that referenced this pull request Feb 11, 2016
…ions

Cleanup duplicate logic and refactor installations.js into InstallationsRouter.
@nlutsenko nlutsenko merged commit 8dcae3d into master Feb 11, 2016
@nlutsenko nlutsenko deleted the nlutsenko.router.installations branch February 11, 2016 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants