How to name a new service in OFN

Tech
1

Follow up from discussion about location of services here where we define most OFN services will end up in app/services (or within engine_name/app/services/. And we will have some folders like app/services/reports, etc.

I create this thread to clarify and get consensus about services naming and structure.

The current list of services we have is this:

  • cart_service
  • create_mail_method
  • create_order_cycle
  • embedded_page_service
  • line_item_syncer
  • order_cycle_form
  • order_factory
  • order_syncer
  • order_update_issues
  • reset_order_service
  • restart_checkout
  • search_orders
  • subscription_estimator
  • subscription_form
  • subscription_validator
  • subscriptions_count

We should decide how to name a service.
In 16 services, we have 6 different approaches here!!!

  1. named service - pattern <resource.>_<actor.>
    • examples: subscription_estimator, subscription_validator, order_syncer, line_item_syncer, order_factory
  2. named action - pattern <action.>_<resource.>
    • examples: create_mail_method, create_order_cycle, restart_checkout, search_orders
  3. resource service - pattern <resource.>_service
    • examples: cart_service, embedded_page_service
  4. named action service - pattern <action.>_<resource.>_service - a mix of 2 and 3
    • examples: reset_order_service
  5. named action with resource first - pattern <resource.>_<action.> - a variation on 2 with resource name first and then action name
    • examples: order_update_issues, subscriptions_count
  6. resource - pattern <resource.> - like in option 1, no indication of action allows for more than one action
    • examples: order_cycle_form, subscription_form

It’s a mess :smiley:

I believe we can accept two of these but not more than 2. Let’s choose?

Additionally there is the question of what is the method name inside. We have some services where we use the method call(). We can 1. be flexible and allow both call() and something else, or 2. we can disallow call or 3. we can enforce call on most services.

.
My opinion:
I accept the (new for me) fact of only having single action services which means excluding options 1, 3 and 6 as long as we keep the resource name in the beginning of the file name (so different services/actions on the same resource stay together in the list), which means excluding 2 and 4. This is my logic to vote for one single option acceptable which is 5.
Re the method name, I don’t like the service.call() pattern… I am not sure what’s the best convention here.

What do you guys think? :smiley:

2

Thank you Luis, this is a great summary. I’m not exactly sure which one is my favourite, but there are some I dislike. Maybe we can reduce the options and then discuss them in more detail.

    1. named action like search_orders: I don’t like how different actions of the same object are not grouped together in a file list. For example search_orders and update_orders are in different places. We would have similar actions together though: search_line_items and search_orders would be together, but that is not a benefit to me.
    1. resource service like cart_service: The suffix _service is redundant. The only content in the name is the name of the resource. So what’s the difference to the model? We need to put the purpose of the service into the name.
    1. named action service like reset_order_service: The suffix _service is redundant again. If we omit it we have the same as number 2 named action.
    1. resource like subscription_form: It smells like a model. If it’s a service, it should be about an action. If it’s about holding data, it’s a model.

Qualities I like:

  • Describing the action or field of actions, because services are about actions.
  • Grouping similar files together, for example subscription_estimator and subscription_validator are close together in the file list.
  • Describe the domain of the service instead of the resource. For example, I prefer stock_updater over variant_updater or variant_stock. A StockUpdater could actually change the stock for products or stock items as well. These feature would be nice together.

So after writing this, I do have a favourite: named service
And as an alternative I like 5. resource named action for single method services that we extract from a model. It’s one step in the refactoring towards the named service that can combine several methods or be applicable to several models.

1 Like
3

All controllers do have the _controller and from outside, for example, when a service is used in a controller, having cart.new or cart_service.new is very different.

This makes me think of a 7th option, a mix between 4 and 5:
7. named action service with resource first** - pattern <resource.>_<action.>_service
- example: order_reset_service or subscription_count_service, mail_method_create_service

I prefer smaller services. I prefer resources rather than domains, for simplicity. We can keep all resources of the same domain together under a folder, so we can have /app/services/stock/variant_updater, same for subscriptions.

Yes, services are mostly about actions BUT they should do DTO stuff, we shouldnt put data logic in the models just because it’s data, specially if it doesnt depend on the ORM mappings.

That’s why I think we need to keep 6 for these case: 3222 Improve stock levels calculation code on cart populate and add specs to test handling of variant overrides by luisramos0 · Pull Request #3230 · openfoodfoundation/openfoodnetwork · GitHub
how would you call this service?

.
I keep my vote on 5. 1 and 7 are ok.

4

I would call that one StockLevelAggregator. But I’m also wondering if it should be a data structure:

stock = StockLevelCollection.new(order, requested_variant_ids)
stock.to_hash
stock.to_json

It could use proper OOP data structures instead of hashes and make the intention of wrap_json_infinity even more clear.

That infinity problem should be solved in a JSON encoding library though. It’s independent of OFN.

5

Yes, I agree the hashes are very bad, but without changing the code, what name would you give it?

  1. stock_levels_aggregator
  2. aggregate_stock_levels
  3. stock_levels_service
  4. aggregate_stock_levels_service
  5. stock_levels_aggregate
  6. stock_levels
  7. stock_levels_aggregate_service

in this case we could have resource as stock_levels only or with variant_stock_levels.

I’ll do the create_mail_method:

  1. mail_method_factory
  2. create_mail_method
  3. mail_method_service
  4. create_mail_method_service
  5. mail_method_create
  6. mail_method
  7. mail_method_create_service

and the * reset_order_service:

  1. order_resetter
  2. reset_order
  3. order_service
  4. reset_order_service
  5. order_reset
  6. order
  7. order_reset_service

:smiley:

6

I don’t have a strong opinion on this. I just want us to agree on a style guide and move on. I’d rather focus on have proper services, and any other kind of abstraction, and not get too caught up with naming conventions. After all, they are just that, conventions.

What I can provide is the long-established conventions within the Rails community but I think that fits into future discussions if needs be.

Without further ado my preference is to choose any of the permutations of action (verb) plus a resource. Between them, the differences are negligible in terms of understanding. That is 1, 2 (how I personally used to name single-responsibiliy services), 4, 5 and 7.

I think others, fail to convey the purpose of the class an as being vague, they can easily lead to bloated classes without a single responsibility.

7

Thanks for your feedback!

So, from the 7 options…
I’d go for not using _service, that kills options 3, 4 and 7.
As discussed above option 6 (only resource name) could fall into one of the other categories but I think there should be space for non-ORM Data Objects in /services, for example, as mail_method or models/product_import/spreadsheet_data. This can be a separate topic for the future.

From the rest, I’d vote with Maikel for keeping the resource name in the beginning of the file name, that kills option 2.

that leaves us with option 1 and 5, basically, either resource/actor and resource/action. I think we can leave this open for each situation.

This would be the convention:
Services must start with the resource name (this resource can be a model or something not so specific) and be followed by the action they perform (reset, search, create) or the actor (aggregator, validator, syncer, etc).

Are you guys ok with this?

This would mean the following renames for existing services:

  • cart_service - - cart_populator
  • create_mail_method - - mail_method_create
  • create_order_cycle - - order_cycle_create
  • embedded_page_service - - page_embed
  • line_item_syncer :+1:
  • order_cycle_form :+1:
  • order_factory :+1:
  • order_syncer :+1:
  • order_update_issues :+1:
  • reset_order_service - - order_reset
  • restart_checkout - - checkout_restart
  • search_orders - - orders_search
  • subscription_estimator :+1:
  • subscription_form :+1:
  • subscription_validator :+1:
  • subscriptions_count :+1:
8

I’m happy with that. :+1:

9

I’m happy with the combination of 1 and 5. :+1:

10

About namespaced services…

The Enterprise Fee Summary is read-only and isolated, so it was easier to group its services together into a folder. But it gives an example of where the services names being only <actor> or <action> makes sense.

These are all under OrderManagement::Reports::EnterpriseFeeSummary. The convention above would mean that these services will have to be renamed:

  • Authorizer → ReportAuthorizer
  • Parameters → ReportParameters
  • Permissions → ReportPermissions
  • ReportService → ReportGeneration
  • Scope → ReportScope
  • Summarizer → ReportSummarizer

For this example, the resource is already the namespace, and it’s a shame to have to add <resource>_.

I think we should make exceptions for namespaced services like this, i.e. if there is no name, then the service pertains to the namespace resource. Of course we could still specify another resource when it makes sense.

What do you think?

Then for the services above, only ReportService would have to be renamed, maybe to Factory or Generator.

11

I agree. We don’t need to duplicate the resource name if it’s in the namespace already. :+1:

12

YES! consensus!! We all agree here, including the namespace. I guess you @luisramos0 will go ahead and add the convention in the wiki.

13

yes Kristina, that’s a valid exception. And that with time and more namespaced services, it can become the norm.
nice, thanks guys.
I’ll add this to the wiki.

1 Like