move API call from controller to model in rails

Question

I am making a Book Club app. Users can upload books that they would like to read along with others. I'm bringing in the book info from Google Books Api and currently have the API call in the controller. I know this is ugly and unrailsy but am having trouble making it work from the model. What would you suggest as the best refactor to make this cleaner?

new.html.erb -from books

  <%= form_for :book, url: books_path do |f| %>
    <fieldset>
      <h1 class="text-center">Add A Book</h1>

      <div class="form-group">
        <label class="col-md-4 control-label" for="name">Title</label>
        <div class="col-md-8">
          <%= f.text_field :title, required: true, class: "form-control" %><br>
        </div>
      </div>

      <div class="form-group">
        <label class="col-md-4 control-label" for="genre">Genre</label>
        <div class="col-md-8">
          <%= f.select :genre, [["Sci-fi", "Sci-fi"], ["Fantasy", "Fantasy"], ["Comic", "Comic"], ["Manga", "Manga"]], class: "form-control" %>
        </div>
      </div>

      <div class="form-group">
        <div class="col-md-12">
          <%= f.submit "Create Book", class: "btn btn-success" %>
        </div>
      </div>

    </fieldset>
  <% end %>

books_controller.rb

def create
  @user = current_user
  find_book
  redirect_to root_path
end

require "openssl"
OpenSSL::SSL::VERIFY_PEER = OpenSSL::SSL::VERIFY_NONE

def find_book
  tempBook = params[:book][:title]
  tempGenre = params[:book][:genre]
  url = "https://www.googleapis.com/books/v1/volumes?q=" + tempBook + "&key=secret_key"
  uri = URI(url)
  response = Net::HTTP.get(uri)
  book_data = JSON.parse(response)    
  b = Book.new
  b.user_id = @user.id
  b.title = book_data["items"][0]["volumeInfo"]["title"]
  b.genre = tempGenre
  b.author = book_data["items"][0]["volumeInfo"]["authors"][0]
  b.publisher =  book_data["items"][0]["volumeInfo"]["publisher"]
  b.publication_date =  book_data["items"][0]["volumeInfo"]["publishedDate"]
  b.synopsis =  book_data["items"][0]["volumeInfo"]["description"]
  b.image = book_data["items"][0]["volumeInfo"]["imageLinks"]["thumbnail"]
  @book = b.save
end

book.rb

class Book < ActiveRecord::Base
  belongs_to :user
  has_many :reviews

  def set_user(user)
    self.user_id = user.id 
    self.save    
  end

end

It works this way, but is ugly and I should hide my key, instead of leaving it open.

I tried putting the function in model and declaring the titles and genres as variables in the show method, but they weren't passed into the model so it didn't work.

Thanks!

Here's the code I tried, but it didn't work. the @tempBook is nil so it messes up the model. I'm assuming that's cause the model is running prior to getting the variable?

book.rb

class Book < ActiveRecord::Base
  belongs_to :user
  has_many :reviews

  def set_user(user)
    self.user_id = user.id 
    self.save    
  end

  require "net/http"
  require "json"

  require "openssl"
  OpenSSL::SSL::VERIFY_PEER = OpenSSL::SSL::VERIFY_NONE

  def self.find_book
    url = "https://www.googleapis.com/books/v1/volumes?q=" + @tempBook + "&key=SECRET_KEY"
    uri = URI(url)
    response = Net::HTTP.get(uri)
    book_data = JSON.parse(response)    
    b = Book.new
    b.user_id = @user.id
    b.title = book_data["items"][0]["volumeInfo"]["title"]
    b.genre = @tempGenre
    b.author = book_data["items"][0]["volumeInfo"]["authors"][0]
    b.publisher =  book_data["items"][0]["volumeInfo"]["publisher"]
    b.publication_date =  book_data["items"][0]["volumeInfo"]["publishedDate"]
    b.synopsis =  book_data["items"][0]["volumeInfo"]["description"]
    b.image = book_data["items"][0]["volumeInfo"]["imageLinks"]["thumbnail"]
    @book = b.save
  end

end

books_controller.rb

  def create
    @tempBook = params[:book][:title]
    @tempGenre = params[:book][:genre]
    @user = current_user
    Book.find_book
    redirect_to root_path
  end

Answer 1

Step 1 to simplify code and make it more readable:

book_data = book_data["items"][0]["volumeInfo"]
# then you can write the shorter version:
b.title = book_data["title"] # etc

Step 2: secret keys are often best served as environment variables Google for how to set up an environment variable on your platform.

The way to use it would be:

# Note that I've also added 'string interpolation' here
# (google that phrase to find out more)
url = "https://www.googleapis.com/books/v1/volumes?q=#{tempBook}&key=#{ENV['BOOK_FIND_SECRET_KEY']}"

Note: these are small things to change.. I'd like to help more, but need more detail on what went wrong when you tried it out in the model (see comments for questions).

Step 3 @variables shouldn't be used (except when they should).

@variables are special... we use them in the controller to pass values to the view - that's because there's a kind of magic handover of data from controllers to views. But in all the rest of our ruby code - it's better to pass data as actual arguments to methods. eg if your book model has a method called find_book - instead of using @tempBook, you'd have an argument called book_name or similar. I'd also break that one big method up into smaller ones - each of which does one specific thing. Here's an example that just modifies your existing code:

# Note: generally it's accepted practice to put these includes
# at the top of the file outside of the class definition
require "net/http"
require "json"

require "openssl"

class Book < ActiveRecord::Base
  # Store the secret key in this constant so you don't have to 
  # keep typing the ENV-var name each time you use it
  SECRET_KEY = ENV['BOOK_FETCH_SECRET_KEY']

  belongs_to :user
  has_many :reviews

  # I don't know what this is, so I've left it...
  # but it probably shoudln't go here...
  OpenSSL::SSL::VERIFY_PEER = OpenSSL::SSL::VERIFY_NONE

  # Make the book-url into a method - abstract out this complexity 
  # so the code for the main find_book method is much easier to read.
  # Notice how we pass in the book_name as an argument
  def self.book_url(book_name)
    "https://www.googleapis.com/books/v1/volumes?q=#{book_name}&key=#{SECRET_KEY}"
  end

  # Fetching the book via the URL is kind of a stand-alone thing to do
  # so it's easy to turn it into a method - that just accepts the 
  # book title, and returns the parsed book data
  def self.fetch_book_data(book_name)
    uri = URI(book_url(book_name))
    response = Net::HTTP.get(uri)
    JSON.parse(response)    
  end

  # and here's what's left for find_book
  # note how we name and use the arguments
  def self.find_book(user, book_name, genre)
    book_data = fetch_book_data(book_name)
    the_book = book_data["items"][0]["volumeInfo"]
    b = Book.new
    b.user_id = user.id
    b.title = the_book["title"]
    b.genre = genre
    b.author = the_book["authors"][0]
    b.publisher =  the_book["publisher"]
    b.publication_date =  the_book["publishedDate"]
    b.synopsis =  the_book["description"]
    b.image = the_book["imageLinks"]["thumbnail"]
    b.save
    # return the new book at the end of the method
    b
  end    
end

Now in the controller, we can call this code very simply:

def find_book
  # We just call the method on Book - passing in our params as the arguments
  @book = Book.find_book(current_user, params[:book][:title], params[:book][:genre])
end

Note: I really like the other Answer's idea of having a GoogleBook class that does the fetching - you can do that and put his fetching code into fetch_book_data (or equivalent) for better separation of concerns.

Answer 2

You should extract the google code out to some class that goes in the lib directory, the places the instantiation of an instance of that class into the book model then call the find_book method there. You can put the configurations keys for the api into a yml file.

#lib/google_book.rb
class GoogleBook
  def initialize(info)
  end

  def find_book
    url = "https://www.googleapis.com/books/v1/volumes?q=" + tempBook + "&key=secret_key"
    uri = URI(url)
    response = Net::HTTP.get(uri)
    book_data = JSON.parse(response) 
    #rest of code
  end
end

#book.rb
class Book < ActiveRecord::Base
  belongs_to :user
  has_many :reviews

  def set_user(user)
    self.user_id = user.id 
    self.save    
  end

  def retrieve_google_book
    google_book = GoogleBook.new(some_params)
    google_book.find_book
  end

end