Skip to content

Conversation

@guidomaiolams
Copy link

No description provided.

Copy link
Contributor

@Dongata Dongata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not bad, but please try to maintain the routes as restfull as possible

@AlphaGit
Copy link
Member

AlphaGit commented Mar 7, 2019

Hey @guidomaiolams! Thanks for the ticket, here are a few suggestions for next time:

  • Make sure to include a description on what this PR is doing. We do have a styleguide about this kind of stuff. Otherwise, reviewers have no clear context on what the intention is. (I'll share it here later, it seems it wasn't public.)
  • You did reference the ticket this solves (thanks!) but the ticket itself doesn't have much information either. Consider expanding it with details. This is specially important in open source projects like this one.

On the changes:

  • I noticed that several lines changed but they seem to only have whitespace changes -- consider avoiding changes that really do nothing, unless you're actually changing them on purpose.
  • Creating the interface so far was a good improvement, but without a base class, the behavior is still spread around across services. Maybe creating a ServiceBase class too? I think it would be a good idea.

On that design:

  • If you take the suggestion above, consider also moving the tests to the base class. Also, consider alternatives, maybe abstraction is not he best approach and we could have composition of services?
  • Services may do different things, one of them might be retrieving stuff from DB. Since these ones rely on context being available, a better name might be ContextServiceBase? This way, people extending from this project have it clear that it only applies to such cases.

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