Opinions needed for my code refactoring

Ruby on Rails
1

Hello,

I took Intermediate Rails and could follow it all while Matt was explaining.

But after certain amount of refactoring in a file I could not think anything further.

I want to refactor filter_by_species method from the following controller file. The resource has only controller, no model. It is the code about getting moving average calculated from the reports found.

The objective of the question is to get hints/suggestion/guidance about how I can go about it from the people know more than me and not someone else do my job.

Here is the Github link of the file: currents/maps_controller.rb at master · tannakartikey/currents · GitHub

class MapsController < ApplicationController
  before_action :authenticate_user!
  def index
  end

  def create
  end

  def show
  end

  def filter_by_species

    @location = Location.find(params[:location]) unless params[:location].blank?
    @lreports = []
    Location.all.each do |location|

      reports = location.reports.where(species)

      avgrep = reports.where(:date => 1.week.ago..Date.today).order(date: :desc)

      prevavgrep = reports.where(:date => 8.days.ago..1.day.ago)

      movavg = movingavg(avgrep,prevavgrep)

      @lreports.push(
        location:location,
        reports: userreport(avgrep),
        cfile: one_locations_json(location),
        movingavg: movavg[:movingavg],
        color: movavg[:color]
      )
    end
    render json: @lreports
  end

  private

  def species
    return nil if params[:species] = "Any"
    return "species_id = #{params[:species]}"
  end
  def movingavg(avgrep,prevavgrep)
    movingavg = 0
    prevmovingavg = 0
    movingavg = avg(avgrep,1.week.ago.to_date,Date.today-1) unless avgrep.blank?
    prevmovingavg = avg(prevavgrep,1.week.ago.to_date-1,Date.today-2) unless prevavgrep.blank?
    puts "----moving average",  movingavg
    puts "----previous moving average",  prevmovingavg
    #  n = avarray.size
    # sum_sqr = avarray.map {|x| x * x}.reduce(&:+)
    # sum_sqr = 0 if sum_sqr.to_f.nan? || sum_sqr.nil?
    # movingavg = 0 if movingavg.to_f.nan?
    # std_dev = Math.sqrt((sum_sqr - n * movingavg * movingavg)/(n-1))
    std_dev = movingavg - prevmovingavg
    if std_dev > 1
      color = "#FF3E38"
    elsif std_dev > 0
      color = "#C1AF6A"
    else
      color = "#4562A8"
      # else std_dev < 0
      #   color = "#940CE8"

    end
    @movingavg = {movingavg: movingavg,color: color }
    @movingavg
  end
  def avg(avgrep,startdate,enddate)
    avggroup = avgrep.group('date').average('catch_keepers')
    puts "Average function: "
    avarray = (startdate..enddate).map {|date|
      if avggroup[date]
        avggroup[date].to_f
      else
        0.to_f
      end
    }
    movingavg = avarray.inject{ |sum, el| sum + el }.to_f / avarray.size
  end
  def userreport(reports)
    @rep = []
    i = 0
    reports.each do |rep|
      puts "@rep user", rep.user.inspect
      puts "@rep user", rep.user.try(:vessel_name)
      puts "@rep user3"
      @rep.push({rep: rep, vessel_name: rep.user.try(:vessel_name)})
    end
    @rep
  end
  def one_locations_json(location)
    puts "Entered Locations JSON"
    f = File.read location.coordinate_file.path
    puts "Entered Locations JSON 2"
    # puts "f is #{f.inspect}".green
    # # f = f[0]
    # puts "f[0] is #{f}".blue
    # location_json = JSON.parse(f)
    # puts "location_json is #{location_json}".green
    # return location_json
    puts "f ", eval(f).to_a
    eval(f).to_a
  end

  def report_params
    params.require(:report).permit(:date, :species_id, :general_location, :catch_keepers, :catch_total, :trip_summary, :primary_method, :tide, :weather, :wind, :spot, :picture, :best_bait, :trip_description, :location_id)
  end
  def location_params
    @location = Location.where(params[:short_name])
  end
end

Thank you for your time!

2

I’d would suggest extracting out a service object. Looking at the code, it seems to only depend only two params, location and species, so we would pass them into the service object.

The only thing the controller needs from the service object is lreports, so the interface would then simply be:

def filter_by_species
  render json: FilterBySpecies.new(params[:location], params[:species]).lreports
end

and your service class would be:

class FilterBySpecies
  def initialize(location, species)
    @location = location
    @species = species
  end

  def lreports
    ... # fill in the details
  end
end

Hope that helps. Feel free to follow-up if you’re stuck.

3

My ActiveRecord relationships are not working anymore.

e.g.

Location.all.each do |location|
  reports = location.reports.where(species_id: 3)
end

undefined method `reports' for #<Species::ActiveRecord_Relation:0x007fe219e3ebc8>

I have put filter_by_species.rb under app/models directory for now.

4

I think I got it.

Species.find returns ActiveRecord instance whereas Species.where returns ActiveRecord::Relation object.

My code trying to perform method on Relation object so it was not working.

@andyw8 Thank you for answering and showing your support.