search

Code2Gether-Discord / JokesOnYou

A learning project, A jokes website build as a team project.
12 stars 11 forks source link

feature/84-User-Saved-Joke #169

Closed Niisc closed 2 years ago

HellHunterMax commented 2 years ago

You have some conflicts to resolve too. Please Pull dev and resolve them.

Niisc commented 2 years ago

Good job Really well written! I'm not sold about the own controller/Service/Repo. Repo I understand because it has its own database entry. Service and Controller could be put under User imo.

Hmm, imo its more organized like this @chrisK00 opinions?

chrisK00 commented 2 years ago

Good job Really well written! I'm not sold about the own controller/Service/Repo. Repo I understand because it has its own database entry. Service and Controller could be put under User imo.

Hmm, imo its more organized like this @chrisK00 opinions?

Splitting up the responsibilites is important to follow SRP and provides more decoupling, easier classes to navigate. If we dont do something like this we need to switch over to MediatR. This approach also doesnt break OCP

We shouldnt stick all our logic in one class, and the responsibilites are a bit different. If we want to have more features regarding our jokes, we wouldnt put them in the jokes controller because we would run in to a OverFlowException (bad joke)

So if we think about our consumers they get jokes on api/jokes but to get saved jokes they get them on api/savedjokes - i think this is totally fine but the alternative is to simply extend the jokes controller. So the route would become api/jokes/saved which is a bit easier to keep track on but at the same time we still split up the classes.

The important part is that we dont nest to much because that will cause a very ugly url, but in this case i cannot see what else we would nest after the saved.

HellHunterMax commented 2 years ago

Good job Really well written! I'm not sold about the own controller/Service/Repo. Repo I understand because it has its own database entry. Service and Controller could be put under User imo.

Hmm, imo its more organized like this @chrisK00 opinions?

Splitting up the responsibilites is important to follow SRP and provides more decoupling, easier classes to navigate. If we dont do something like this we need to switch over to MediatR. This approach also doesnt break OCP

We shouldnt stick all our logic in one class, and the responsibilites are a bit different. If we want to have more features regarding our jokes, we wouldnt put them in the jokes controller because we would run in to a OverFlowException (bad joke)

So if we think about our consumers they get jokes on api/jokes but to get saved jokes they get them on api/savedjokes - i think this is totally fine but the alternative is to simply extend the jokes controller. So the route would become api/jokes/saved which is a bit easier to keep track on but at the same time we still split up the classes.

The important part is that we dont nest to much because that will cause a very ugly url, but in this case i cannot see what else we would nest after the saved.

Thank you for the great explanation! This is where I need to learn.