Refactor coordinates class

[egemen-dev]

This PR is for this issue: https://github.com/kasugaijin/baja-pet-rescue/issues/134

I refactored the GoogleMapCoordinates class to add deviations to all location instances, not just for duplicate locations. I also added metadata about the dog to be displayed on the Google Map info window when the pin :pushpin: is clicked.

It's basically returning array of hashes:

 [ { latitude: 123, longitude: 123, dog_name: example_name, breed: example_breed }, ... ]

In my opinion, this class can be extracted using Service Objects or the class can be deleted, and the GoogleMapCoordinates logic can be moved to SuccessesController. This change makes sense because we are now also dealing with dog names and Adoption instances in the GoogleMapCoordinates class, which is not ideal.

Let me know what do you think @kasugaijin. I'm open to suggestions.

[kasugaijin]

Thanks for starting this. It is exactly along the lines of what I was thinking. I agree that this class could be relocated elsewhere. It seems there is a debate in the Rails world about where would be appropriate to keep it, but I am happy with it becoming a service object. Perhaps we can scope it under API i.e., put it in an API folder, app/services/api/<classname.rb> and call it accordingly in the controller. Please make that change or discuss further here if you have other ideas.

I am all for leaving controllers to deal with request logic, so would prefer leaving this logic out of the controller and placing it in its own file, per above.

[egemen-dev]

I will continue working on this. I just changed the title to indicate the work in progress.

[egemen-dev]

@kasugaijin I added the service actor gem to separate the logic and made the necessary changes as we discussed above. I am content with how it came out. Please let me know if there is anything you want to add or ask.

[kasugaijin]

Thank you for making these changes. It's in the direction I was thinking, however, I have a couple of thoughts about this approach and how we might approach it differently.

• what was behind the thinking of using a gem to run service objects? I think there is a better case to create and manage our own service objects. We already have the app/services folder and we can locate this new class to handle the Google maps logic. The only thing we need to make sure is that that directory is auto-loaded at app init so we can access that class throughout the app. If I recall correctly, app scoped directories are autoloaded, so you will be able to call whatever class you add there per its namespace i.e., if we have it in app/services/api/google_maps.rb we can access it via API::GoogleMaps namespace.
• I am not a huge fan of the namespace used as it is not very intuitive for me app/actors. This directory is not intuitively the first place I would look for a service object doing Google Map prep work. It seems to be inheriting a naming convention put forward by the gem, and it is using a somewhat contrived name for the purposes it is achieving.
• I think a good practice is to not use gems when you don't need to. Sometimes they are great in adding a ton of functionality off the bat, in this case, there's nothing we cannot do without pure Ruby.
• Using this gem will force us to learn and stick to its conventions, which are not as intuitive as pure Ruby, in my opinion.
• The more dependencies you have, the more dependencies that can cause issues.

What I was thinking was moving the logic to a class called GoogleMapsDataBuilder that lives in app/services/api/google_maps_data_builder.rb . To me that is an intuitive location for this PORO to live. If I was to open this app up for the first time tasked with finding the logic that creates data for google maps, that would be the first place I would look based on its naming. Then we can call API::GoogleMapsDataBuilder.method_name in the controller. We can maintain the .call method if you prefer (I think that's an older rails convention?) and structure the service object accordingly.

Please let me know your thoughts on this! :)

[egemen-dev]

Thank you for making these changes. It's in the direction I was thinking, however, I have a couple of thoughts about this approach and how we might approach it differently.

* what was behind the thinking of using a gem to run service objects? I think there is a better case to create and manage our own service objects. We already have the `app/services` folder and we can locate this new class to handle the Google maps logic. The only thing we need to make sure is that that directory is auto-loaded at app init so we can access that class throughout the app. If I recall correctly, `app` scoped directories are autoloaded, so you will be able to call whatever class you add there per its namespace i.e., if we have it in `app/services/api/google_maps.rb` we can access it via `API::GoogleMaps` namespace.

* I am not a huge fan of the namespace used as it is not very intuitive for me `app/actors`. This directory is not intuitively the first place I would look for a service object doing Google Map prep work. It seems to be inheriting a naming convention put forward by the gem, and it is using a somewhat contrived name for the purposes it is achieving.

* I think a good practice is to not use gems when you don't need to. Sometimes they are great in adding a ton of functionality off the bat, in this case, there's nothing we cannot do without pure Ruby.

* Using this gem will force us to learn and stick to its conventions, which are not as intuitive as pure Ruby, in my opinion.

* The more dependencies you have, the more dependencies that can cause issues.

What I was thinking was moving the logic to a class called GoogleMapsDataBuilder that lives in app/services/api/google_maps_data_builder.rb . To me that is an intuitive location for this PORO to live. If I was to open this app up for the first time tasked with finding the logic that creates data for google maps, that would be the first place I would look based on its naming. Then we can call API::GoogleMapsDataBuilder.method_name in the controller. We can maintain the .call method if you prefer (I think that's an older rails convention?) and structure the service object accordingly.

Please let me know your thoughts on this! :)

You are totally right about not adding more dependencies. I just liked using actor gem, it felt so right haha :)

I made some changes about using PORO and removing the gem used :heavy_check_mark:

First, I created an api folder under services like services/api to call our service object like API::GoogleMapsDataBuilder but since SuccessesController is not in the api module like controllers/api/successes_controller.rb, we need to specifically require at the top of the controller or any other file like this: require 'api/google_maps_data_builder. I think it is not very convenient as we might want to use it elsewhere and run into NameError. Keeping it at the app level enables us to use it wherever we want without requiring.

Let me know what you think? @kasugaijin

[kasugaijin]

I have not tested here yet, but if you place the service object in /services without scoping it under /app then yes you will need to require the file at the top of the controller. I agree this is not clean.

I do not think you need to require the file if you scope under /app. If you take a look at the file tree in the app you will see /app/services folder already exists. I was thinking you could put the service object there at app/services/api/google_maps_data_builder.rb and you can namespace it under an API module like

module API
  class GoogleMapsDataBuilder
    # Service object implementation
  end
end

And call it in the controller with API::GoogleMapsDataBuilder

Can you confirm you have tried this? It 'should' work.

[egemen-dev]

I did exactly how you recommended like above. It did not work out and run into NameError like I mentioned it above. If you look at the google_maps_data_builder.rb location you will see I already used app/services folder in the last commit. This version is working like a charm and passes all the tests. You can switch to this branch and try it out yourself to see it does not work without explicitly requiring at the top of the controller when we put the file under app/services/api

[kasugaijin]

Ok I know what's going on now! I tested it out locally in the console. It turns out my recommendation for namespacing in api is a bad one as this module is already in use by Rails...which in retrospect is obvious as it is so generic. So, we can come up with a more unique namespace name for it and place it under that module. Then we can call module::service_object in the controller.

I like what you have done, and it works right now just fine. But, in terms of long-term, it's easier to namespace now than go back and organize things later when it is needed. How about namespacing based on the integration, in this case GoogleMap so it will be GoogleMap::GoogleMapsDataBuilder living in app/services/google_map ? I like that. I am open to suggestions!

[egemen-dev]

Ok I know what's going on now! I tested it out locally in the console. It turns out my recommendation for namespacing in api is a bad one as this module is already in use by Rails...which in retrospect is obvious as it is so generic. So, we can come up with a more unique namespace name for it and place it under that module. Then we can call module::service_object in the controller.

I like what you have done, and it works right now just fine. But, in terms of long-term, it's easier to namespace now than go back and organize things later when it is needed. How about namespacing based on the integration, in this case GoogleMap so it will be GoogleMap::GoogleMapsDataBuilder living in app/services/google_map ? I like that. I am open to suggestions!

Now we know why and congratulations o that! I did not know it was already in use by Rails.

I think we can use GoogleMap:: module name as it is very obvious and I like that.

For the class name, my suggestions would be to rename it from GoogleMapsDataBuilder to DataBuilder and use it like GoogleMap::DataBuilder

What do you think?

Edit: I pushed a commit to reflect changes on the topic. (https://github.com/kasugaijin/baja-pet-rescue/pull/160/commits/703648e739f03457c83420fb3a32c73d408396de)

[kasugaijin]

Great, we finally got there! :man_dancing: I like your suggestion for naming the PORO DataBuilder so please make the change and I think it'll be ready for merging.

[egemen-dev]

Great, we finally got there! man_dancing I like your suggestion for naming the PORO DataBuilder so please make the change and I think it'll be ready for merging.

Ready :confetti_ball: :confetti_ball: @kasugaijin

[kasugaijin]

Thank you for seeing this one through @egemen-dev So, this refactor produces deviated coordinates in a much simpler way and prepares addition of meta data like dog name to the google map pin, per this issue: https://github.com/kasugaijin/baja-pet-rescue/issues/158 . You are currently assigned. Are you still good to go on this one, too? If so, please make the changes and update the integration test accordingly and I will await PR. No rush.

Thanks Ben