Skip to content

CountryController: remove dependency on SeriesService by moving showInfoBySlug() method #1052

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

Closed
0pdd opened this issue Jun 16, 2019 · 5 comments
Assignees
Milestone

Comments

@0pdd
Copy link

0pdd commented Jun 16, 2019

The puzzle 927-4ad1152b from #927 has to be resolved:

// @todo #927 CountryController: remove dependency on SeriesService by moving showInfoBySlug() method

The puzzle was created by Slava Semushin on 16-Jun-19.

@0pdd 0pdd added the techdebt label Jun 16, 2019
@0pdd 0pdd mentioned this issue Jun 16, 2019
14 tasks
@php-coder php-coder added this to the 0.4.2 milestone Sep 9, 2019
@php-coder php-coder assigned mukeshk and unassigned mukeshk Sep 17, 2019
@mukeshk
Copy link
Contributor

mukeshk commented Sep 18, 2019

So in CountryController - showInfoBySlug method internally uses -> seriesService.findByCountrySlug .
When we say we need to move to showInfoBySlug method.
Do we need to move the showInfoBySlug method to SeriesService?

Can you elaborate? Thanks.

@php-coder
Copy link
Owner

Note that you shouldn't work on this issue yet because I've realized that it's not so easy to fix it as I thought before.

So in CountryController - showInfoBySlug method internally uses -> seriesService.findByCountrySlug . When we say we need to move to showInfoBySlug method. Do we need to move the showInfoBySlug method to SeriesService?

The idea was to move the method showInfoBySlug() from CountryController to SeriesController. But I realized that it's not I really want as it won't fix the issue really.

@php-coder
Copy link
Owner

In order to give you overall understanding on what is all about: I'm trying to split everything by features (country, category, series, collection, etc). All the classes that belong to a feature should be placed into the same package.

Also we shouldn't have circular dependencies between packages. For example, package "country" shouldn't depend on a package "series" because the latter already depend on the "country" package. That's why I had idea to move this method to SeriesController because it uses SeriesService and we have a dependency on "series" package.

But I don't like a fact that SeriesController will have an endpoint for "/country/{slug}" URL. It seems a little weird. So, that's why I cancelled a work on this issue for a while.

@php-coder
Copy link
Owner

At this moment, I have an idea to introduce an endpoint on SeriesController and make CountryController to fetch data from SeriesController.

For example, here is how it looks like now:
/country/{slug} -> CountryController -> SeriesService -> SeriesDao

The proposal is to make it looks like:
/country/{slug} -> CountryController -> /series?country=slug -> SeriesController -> SeriesService -> SeriesDao

But it still not perfect as SeriesService/ServiceDao work with country table that doesn't belong to "series" at all. So, another idea is to resolve country slug to country id on CountryController and modify SeriesController to accept countryId instead of country slug:

/country/{slug} -> CountryController -> /series?country_id=$id -> SeriesController -> SeriesService -> SeriesDao

Here is what I'm thinking about now. I'll be happy if you have ideas on that.

@php-coder
Copy link
Owner

Another idea: /country/{slug} should be renamed to /series?country={slug} and this endpoint can be moved to SeriesController then.

@php-coder php-coder self-assigned this Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants