Second opinion needed for my refactoring

Ruby on Rails
1

I refactored a controller to make the code more readable. Please have a look at it before the refactoring:


module Api
  module V1
    class DevicesController < ApplicationController
      def create
        # the params either have a :device_token or a :registration_id
        if device_params['device_token'].present?
          @device = Device.where(device_token: device_params['device_token']).first
        else
          @device = Device.where(registration_id: device_params['registration_id']).first
        end

        add_app_id_if_blank

        # This :create method is used for new_records and
        # for updating existing ones as the 
        # client is deliberately dumb 
        # and just posts to this one route
        if @device == nil
          @device = Device.new(device_params)
          respond_to do |format|
            if @device.save
              format.json { render json: { "link" =>  "#{request.base_url}/api/v1/apps/#{@device.app.guid}/devices/#{@device.id}" }, status: :created }
            else
              format.json { render json: @device.errors, status: :unprocessable_entity }
            end
          end
        else
          respond_to do |format|
            if @device.update_attributes(device_params)
              format.json { render json: { "link" =>  "#{request.base_url}/api/v1/apps/#{@device.app.guid}/devices/#{@device.id}" }, status: 200 }
            else
              format.json { render json: @device.errors, status: :unprocessable_entity }
            end
          end
        end
      end

      private

      def device_params
        params.require(:device).permit(:app_id, :device_token, :registration_id, :identifier_for_vendor, { tag_list: [] })
      end

      def add_app_id_if_blank
        unless params[:device].include?(:app_id)
          params[:device][:app_id] = App.where(guid: params[:app_id]).first.id
        end
      end
    end
  end
end

After my refactoring I had this result:

module Api
  module V1
    class DevicesController < ApplicationController
      before_filter :add_app_id_if_blank, :find_or_create_device
      respond_to :json

      def create
        if @device.update_attributes(device_params)
          render json: { "link" =>  "#{request.base_url}/api/v1/apps/#{@device.app.guid}/devices/#{@device.id}" }, status: @status
        else
          render json: @device.errors, status: :unprocessable_entity
        end
      end

      private

      def device_params
        params.require(:device).permit(:app_id, :device_token, :registration_id, :identifier_for_vendor, { tag_list: [] })
      end

      def add_app_id_if_blank
        unless params[:device].include?(:app_id)
          params[:device][:app_id] = App.where(guid: params[:app_id]).first.id
        end
      end

      def find_or_create_device
        @device = Device.where("device_token = ? OR registration_id = ?", device_params['device_token'], device_params['registration_id']).first
        @status = 200
        if @device == nil
          @device = Device.new(device_params)
          @status = :created
        end
      end
    end
  end
end

I would say I like everything about it but the find_or_create_device method. Do you have an idea how to optimize this method? I don’t need the different @status but I want to represent the correct state for the client. Even if it isn’t neccessary (since it is a dumb client). So what’s your take on this?

Thank you.

2

Seems like you are creating a server to go with the push notification server. I created something similar too :smile:

3

Yes, that´s right. This is a server for push notifications and in-app purchase verification etc.
Any thoughts on the code? :smile:

4

Do u have specs written? If yes then just follow ur specs. I dont like just changing the code just to make prettier

5

The code is tested, that wasn´t the problem here. :smile:
As I wrote I changed the code to make it more readable and easier to understand for future-me or other developers.

Right now I am wondering how to make the last method better. Do you have an idea for that?

6

I am assuming there can be only one device with a device_token or a reg_id. So you dont have to do a where and the first. You can use find. Something like

@device = Device.find_by(device_token:device_params['device_token']) || Device.find_by(registration_id:device_params['registration_id'])

if @device.nil?
   @device = Device.new(device_params)
   @status = :created
end

I am not sure if my code is anywhere better than yours. :slight_smile:

7

What about moving add_app_id_if_blank and find_or_create_device to different classes, and calling those classes from the create action. It should make each class more focused, easier to test, more reusable and overall less complex.

8

I don’t think u need a new class. A class should consist of data and methods that act on it. You are better off creating modules.

9

Using mixins has many problems, mostly because they are a form of inheritance, I’ll quote Ruby Science here:

• They use the same namespace as classes they’re mixed into, which can cause naming conflicts.
• Although they have access to instance variables from classes they’re mixed into, mixins can’t easily accept initializer arguments, so they can’t have their own state.
• They inflate the number of methods available in a class.
• They’re not easy to add and remove at runtime.
• They’re difficult to test in isolation, since they can’t be instantiated.

That’s why you should always prefer composition. Also, moving those methods into different classes does not go against anything of what you said. Moreover, it can be argued that the present class breaks SRP.

10

Oh cool. Thanks @zamith for clarifying that thought. I was myself always confused when to use a new class and when to create a new method.

11

Thanks @Zamith, that sounds like a reasonable idea. I will try that approach.
The only thing that speaks against that is Sandy Metz’ rule number 4:

Controllers can instantiate only one object. Therefore, views can only know about one instance variable and views should only send messages to that object (@object.collaborator.value is not allowed).

Or am I wrong?

Thanks for participating here.

12

If you don’t want to instantiate more than one object in the controller, you can have a service object or facade that does all the “dirty” work, and only instantiate that.

On the other hand, if you relax the rule a bit to be only send one object to the view, you can instantiate other objects in the controller as helpers, an only send the device to the view.

It really depends on how much you feel confortable with moving out of the controller and on the rest of the codebase.

13

Yeah, this mirrors my thoughts.
Thanks very much for this valuable feedback.