Quote list API, CORS and refactor a bit#204
Conversation
ForNeVeR
force-pushed
the
feature/84-list-api
branch
from
693642a to
321ecf4
Compare
ForNeVeR
force-pushed
the
feature/84-list-api
branch
from
321ecf4 to
cbd1661
Compare
|
|
||
| object Quotes extends Controller { | ||
| def getQuote(id: Long) = ActionWithTx { request => | ||
| private def action(responseAction: RequestWithSession[AnyContent] => Result) = ActionWithTx { request => |
There was a problem hiding this comment.
Such a generic name attracts generic code. I suggest to rename it to corsAction or similar. Otherwise this function may grow uncontrollably in the future.
There was a problem hiding this comment.
I have already done that in scope of #94, so I think I'll just merge these pull requests together, and review the result afterwards.
There was a problem hiding this comment.
(done, the function is now called corsActionWithTx)
scalajvm/app/helpers/Cors.scala
Outdated
| @@ -0,0 +1,26 @@ | |||
| package helpers | |||
There was a problem hiding this comment.
"Helpers" and "Utilities" are instances of coincidental cohesion which is the worst possible cohesion according to Larry Constantine https://en.wikipedia.org/wiki/Cohesion_(computer_science)#Types_of_cohesion
I will create an issue to eliminate this package, but for I now I strongly recommend to create cors package and move everything related to CORS there.
There was a problem hiding this comment.
Yep, moved that stuff into cors package.
scalajvm/app/global/Options.scala
Outdated
| @@ -0,0 +1,10 @@ | |||
| package global | |||
There was a problem hiding this comment.
Why is Options controller in global package? Does the global package have a special meaning in Play Framework like Global object? I think I'm missing something here...
There was a problem hiding this comment.
I meant that to be the "global" handler for all HTTP OPTIONS requests. But
- it's not as global anymore, because it handles only
/api/*route - I agree; moved it to
corspackage
|
|
||
| object Quotes extends Controller { | ||
| def getQuote(id: Long) = ActionWithTx { request => | ||
| private def action(responseAction: RequestWithSession[AnyContent] => Result) = ActionWithTx { request => |
There was a problem hiding this comment.
This function looks like something we may want to reuse accross different controllers. Are you sure you want it to be sitting here privately?
There was a problem hiding this comment.
You were right, and it's now reused within Cors trait.
2 participants
Tasks solved
/apiroute (and enable CORS for it)/apiroute (and enable CORS for it)Impact
Close #94.