5
\$\begingroup\$

I did a small sample application for an Interview. Since I didn't got accepted I would like your opinion on how to improve it.

The aux methods are private. I have a usage_common to remove duplication in the usage and usage_instance methods add_item_hash is used because the format written in the database is different from the used in the API response

The start stop and status actions are used to simple operations in EC2 instances

Maybe I should've broken it in services? one for usage, one for processes and one for EC2.

Should send the params already in the format for database and thus eliminate the methods build_usage_hash and build_process_hash

https://github.com/mabranches/monitor_api/blob/master/app/controllers/instances_controller.rb

class InstancesController < ApplicationController
  before_action :set_instance, only: [:show, :update, :destroy]

  def usage
    usage_common(:get, (Time.now - 24.hours).utc.iso8601, Time.now.utc.iso8601)
  end

  def usage_instance
    usage_common(:get_instance, params[:id], (Time.now - 24.hours).utc.iso8601, Time.now.utc.iso8601)
  end

  def processes_instance
    items = ProcessList.get_instance(params[:id])
    process_items = {}
    items.each do |item|
      process_items[item['instance_id']] = item
    end
    render json: process_items
  end

  def create
    usage = build_usage_hash params
    process = build_process_hash params

    Usage.create(usage)
    ProcessList.create(process)

    render json: [usage, process], status: :created
  rescue => e
    Rails.logger.error("Was not possible to update instance data: #{e.message}")
    render json: { error: e.message }, status: :unprocessable_entity
  end

  def processes
    items = ProcessList.get
    process_items = {}
    items.each do |item|
      process_items[item['instance_id']] = item
    end
    render json: process_items
  end

  def stop
    @params = params
    ec2_action(:stop, 'stopped')
  end

  def start
    @params = params
    ec2_action(:start, 'started')
  end

  def status
    @params = params
    ec2_instance do |ec2|
      @status = ec2.state.name
    end
  end

  private

  def usage_common(method, *args)
    items = Usage.send(method, *args)

    result = {}
    items.each do |item|
      add_item_hash(result, item)
    end
    render json: result
  end

  def ec2_instance
    instance_id = @params[:id]

    return render json: {}, status: :unprocessable_entity unless instance_id
    ec2 = Aws::EC2::Resource.new(region: 'us-west-2').instance(instance_id)
    @status = 'unknown'
    begin
      yield ec2
    rescue => e
      Rails.logger.error("Was not possible to execute action #{@params[:action]}: #{e.message}")
    end
    render json: { instance: instance_id, status: @status }
  end

  def ec2_action(method, status)
    ec2_instance do |ec2|
      ec2.send(method)
      @status = status
    end
  end

  def add_item_hash(result, item)
    result[item['instance_id']] ||= Hash.new { |h, k| h[k] = [] }
    result[item['instance_id']]['mem'] << item['mem']
    result[item['instance_id']]['disk'] << item['disk']
    result[item['instance_id']]['cpu'] << item['cpu']
    result[item['instance_id']]['usage_time'] << item['usage_time']
  end

  def build_usage_hash(params)
    instance = params[:instance]
    {
      instance_id: params[:id],
      cpu: instance[:cpu],
      mem: instance[:mem],
      disk: instance[:disk],
      usage_time: Time.now.utc.iso8601
    }
  end

  def build_process_hash(params)
    instance = params[:instance]
    {
      instance_id: params[:id],
      process: instance[:process].try(:values) || []
    }
  end
end
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to CR! You should have edited your previous post to improve your existing question instead of making a new one, the system doesn't like negative user records. Here, have an upvote, to steer yours into positiveness. \$\endgroup\$ – Mathieu Guindon Jun 13 '16 at 17:00
1
\$\begingroup\$

My gut reaction is that there's too much going on in this controller.

By just squinting at the code, I'd say that A) more code should be moved to the model(s), and B) you should be consistent in how your actions work.

For the latter point, some of your actions explicitly render a response, while others are more like aliases for other methods. The latter makes the controller confusing. At first glance I wasn't sure if, say, #usage is an action or a helper method, since I didn't see it explicitly respond to anything. More on the strange mix of methods later.

Also structurally, I don't understand your domain modelling. Namely, why don't you have an Instance model anywhere? That appears to be the primary resource your API handles - your controller is even called InstancesController - but an "instance" is not a first-class model in your system.

Instead you have indirections like Usage and ProcessList, which seems backwards; why not ask an Instance object for its processes and usage? And ask it to start and stop etc.? Behind the scenes it could use the same mix of database lookup and Aws methods, but it'd be bundled up in a simpler interface - again, skinny controller, fat model.

Looking at the code more closely, there's more worrying stuff:

  • You have this:

    def status
      @params = params
      ec2_instance do |ec2|
        @status = ec2.state.name
      end
    end
    
    # ...
    
    def ec2_instance
      instance_id = @params[:id]
    
      return render json: {}, status: :unprocessable_entity unless instance_id
      ec2 = Aws::EC2::Resource.new(region: 'us-west-2').instance(instance_id)
      @status = 'unknown'
      begin
        yield ec2
      rescue => e
        Rails.logger.error("Was not possible to execute action #{@params[:action]}: #{e.message}")
      end
      render json: { instance: instance_id, status: @status }
    end
    

    This is extremely confusing: You have to set the @params instance variable (and hope it contains an id key/value) before you call #ec2_instance. Then you have to pass a block that must set the @status instance variable. And then #ec2_instance renders that status... what?

    You've basically split one task into two tightly coupled, brittle methods. Why? This alone would probably get you rejected!

    Also, from an API standpoint, if status is returned as unknown, I'd like to know why. That information just gets logged on the server, which doesn't help me, the client.

    Again, it'd be much cleaner to have an Instance model that you just call #status on, and have it return the status or raise an exception, which you can then handle appropriately.

  • This sort of thing

    process_items = {}
    items.each do |item|
      process_items[item['instance_id']] = item
    end
    

    could more elegantly be written as:

    process_items = items.each_with_object({}) do |item, hash|
      hash[item['instance_id']] = item
    end
    

    or you could use inject/reduce, or map + to_h. Lots of ways to make this more functional instead of modifying a closed-over variable in a block. Of course, none of this should be done in the controller to being with: it'd make more sense the have ProcessList return the appropriate data structure, especially since you have to do this wrangling in two different actions. And for that you seem to have copy/pasted the code: Bad.

    In fact, I'm not even sure it's doing the right thing in #processes_instance: It creates a hash with the instance_ids as keys, but if you're requesting a specific instance, wouldn't there only be one instance_id meaning the same key gets overridden again and again?

    You also have #add_item_hash which is even worse. An each block that modifies a closed-over hash by forwarding it plus the iterated item to a completely different method? Ew. Why doesn't Usage just produce the proper data structure?

  • You have this before_action, but don't actually have those actions in your controller - and the filter method doesn't exist either!

    before_action :set_instance, only: [:show, :update, :destroy]
    

    This is just sloppy.

    Looking in your routes.rb, you have a bug too: Two routes matching get 'instances/usage' but pointing to two different actions. So your API seems to be broken. Haven't looked at your test suite, but it should have revealed this.

  • Configuration? What if I'm not looking for something in the us-west-2 region? What if I want more than 24 hours of usage data? Both those things are just magic values right now.

All in all, the code has problems on both the macro and micro level. I'm afraid there are good reasons you didn't get accepted.

\$\endgroup\$
1
\$\begingroup\$

I see two things wrong with this controller.

  1. It's too 'fat'. You're doing way too much in one controller.
  2. It's not RESTful.

In this case, those two problems are much the same. Without reading too much into the code, it looks like you could break that controller up into 3 controllers, UsageController, InstanceController, Instance::PowerController. I'm not certain about the last one, you may find the Instance's power state not to be a RESTful resource.

Here's an example: https://stackoverflow.com/documentation/ruby-on-rails/4373/method-on-a-controller-that-s-not-part-of-the-default-five#t=201608110000585779419

As well though, I'd say the building of the hashes should be encapsulated by a model.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.