Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

Adding Service Layer Implementation #164

Closed
wants to merge 15 commits into from
Closed

Conversation

podoler
Copy link

@podoler podoler commented Aug 30, 2014

Separate controller logic into two separated layers:

  1. Controller layer - to handle http transport layer logic
  2. Service layer - to handle business logic

@rschwabco
Copy link
Member

I reviewed this PR, and I appreciate the effort you put into this. Thank you for your awesome work. That said, I don't know if I'm fully on board with this PR yet. I'd like to bring this up for discussion - As with other PRs we've seen, this falls in the category of "implementation specific" - this might make perfect sense in your project, but this degree of separation might always be needed. What do you think?

@podoler
Copy link
Author

podoler commented Oct 21, 2014

Thank you for your review,
I believe this layer should always be implemented,
I think mean should give the solution for one as a good infrastructure to start with with the ability to scale and grow,
Ad I see it there should be couple of layer's which few of them are:

  1. Controller - Should handle the http layer, should not familiar with the business logic of the module
  2. Services - Should handle the business logic of the specific module
    This separation will give the ability to reuse code and allow scalability for projects and should be continued integrated in other modules as well....

@ojhaujjwal
Copy link

+1 for me!

@ilanbiala
Copy link
Member

I don't think a "service layer" is a good idea, too separated, too much importing and requiring, and it just moves SLOC around to places most people aren't familiar with.

@NeverOddOrEven
Copy link
Contributor

I like the idea of having a service layer personally, as most projects I have been on avoid business logic in view controllers, though I can see how a 1-to-1 controller/service abstraction may seem trite. That said, it could make sense to do this as a sub-generator that can add services to module(s).

@ilanbiala
Copy link
Member

@lirantal @amoshaviv @roieki @NeverOddOrEven this really just moves the code to another file, which I don't see as necessary. Do you guys have any strong preference for this change?

@ilanbiala ilanbiala self-assigned this Jan 25, 2015
@podoler
Copy link
Author

podoler commented Jan 25, 2015

When the project is small, this might seem as a redundant overhead,
but when things start to grow and get more volume, this abstraction will be necessary for code reuse, better testing, easy to read, decoupled code etc.

@ilanbiala
Copy link
Member

@podoler right, but we shouldn't force this on users, especially with such a small codebase. Also, the service layer isn't really decoupling or making this easier to test, it just splits it into 2 functions basically...

@podoler
Copy link
Author

podoler commented Jan 25, 2015

what @NeverOddOrEven suggested sounds right, if you do not want to force this upon users.
As for the decoupling and testing, from my experience, it really helps... We could reuse the service layer more easily and testing became straight forward...

@ilanbiala
Copy link
Member

This seems like too much maintenance even for a sub-generator, I would just add a page to the Wiki saying here's a suggestion for larger projects with a code sample.

@lirantal
Copy link
Member

honestly speaking this is quite a large PR and I'm unable to review it properly in terms of schedule. I would anyway suggest to do this PR with the 0.4 branch, if at all.

@ilanbiala
Copy link
Member

@lirantal the main idea is whether to move the actual logic into a separate method, and then call that function then do res.json(). The issue I have is that it doesn't suit most users' needs and it just moves code to a different folder, which I feel is unnecessary.

@lirantal
Copy link
Member

@podoler looks like a considerable PR but we can't merge it into master sadly.
Is this still relevant for 0.4.0?

@codydaig
Copy link
Member

codydaig commented Aug 2, 2015

Since there is no active discussion, and not many people are onboard with this idea, I'd suggest we close. If someone wants to bring this back up for discussion, it would be done against the latest version.

@lirantal
Copy link
Member

lirantal commented Aug 2, 2015

@podoler this an old PR that we can't merge but if you'd like to keep up with PR contributions to the project we'd love to hear more from you.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants