Idiomatic Ruby if/else in a Sinatra route

Question

Apologies if this question doesn't fit the format here, seemed more appropriate here than SO due to the more nebulous nature of the question.

Think mini tinyURL clone: POST to service to save the url + hash, then look up the mapping when visiting /anything. It's been a while since I've touched Ruby, and the resulting code snippet is not easy on the eyes:

require 'sinatra'
require 'mongoid'
require 'digest'

class MinifiedUrl
  include Mongoid::Document

  field :code, type: String
  field :original_url, type: String
end

# Redirect to the actual url, like tinyurl.com/abcdefg
get '/:code' do
  link = MinifiedUrl.where(:code => params[:code]).first # .first is falsy if there is no mapping
  if link
    puts "Found minified link"
    redirect link[:original_url], 302
  else
    puts "No minified link"
    redirect '/404', 404
  end
end

# Create a mapping from a POST request like { url: 'https://google.com/ '}
post '/' do
  incoming_url = params[:url]

  link = MinifiedUrl.where(:code => params[:code]).first 
  # Reassign link if link is falsy
  link = MinifiedUrl.create(:target_url => incoming_url, :code => Digest::MD5.hexdigest(incoming_url)[0..5]) unless link

  link[:slug]
end

Is there an obvious way to make the code more idiomatic? My concern is multiple calls to MinifiedUrl model / reassigning value of link. I can't put my finger on it, but it doesn't feel ruby-like.

Answer 1

I agree that re-assigning variables is not great. The most 'obvious' way to improve this is to extract a helper method like

def fetch_link
  if (existing_link = MinifiedUrl.where(:code => params[:code]).first)
    existing_link
  else
    MinifiedUrl.create(:target_url => params[:url], :code => generate_code)
end

def generate_code
  Digest::MD5.hexdigest(incoming_url)[0..5]
end

or if you prefer or memoization

def fetch_link
  return existing_link if existing_link.present?
  
  MinifiedUrl.create(:target_url => params[:url], :code => generate_code)
end

def existing_link
  @existing_link ||= MinifiedUrl.where(:code => params[:code]).first
end

def generate_code
  Digest::MD5.hexdigest(incoming_url)[0..5]
end

Another approach could be to implement a class method on your model which does it for you like

class MinifiedUrl
  def self.find_or_create_by(original_url:, code:)
    if (existing_link = MinifiedUrl.where(:code => code).first)
      existing_link
    else
      MinifiedUrl.create(:target_url => original_url, :code => generate_code)
    end
  end
end

post '/' do
  incoming_url = params[:url]

  link = MinifiedUrl.find_or_create_by(url: incoming_url, code: params[:code])

  link[:slug]
end

## Edit

Adding some more thoughts about error handling here.

URL

There are several things which can go wrong with the URL like invalid or already taken.

The invalid case I would cover with a validation but there is not much what you can validate here to be honest except the format. Mongoid comes with ActiveSupport validations so you could do something like this.

class MinifiedUrl
  validates_format_of :original_url, with: /regex/
end

MinifiedUrl.create(url: 'invalid').valid?
# false

It can be that your URL is already taken, e.g. somebody else already added this URL. What do you want to do in this case? Give an error or allow it? I think it's a valid use case to allow the same URL twice with a different code (you don't want to have the same code as e.g. you want to have a visit counter as well). If you want to allow the same URL twice, your approach with the MD5 hash does not work anymore as it would be the same obviously so in this case you should use something more random.

The hex digest is anyway a problem in case of collisions you would fail at the moment. I think a better approach is to just generate a random code in a loop.

def generate_code
  100.times do
    code = SecureRandom.hex
    break code unless MinifiedUrl.find_by(code: code)
  end
end

I'm also wondering why your POST does accept a code parameter but I don't know your user interface, I just found it a bit confusing.

I wouldn't be too concerned about DB not available and would not put any code handling this in my application to be honest. If you need this safety you should have DB replicas which can handle if a node goes away.

I would also calculate if a 5 digit code is big enough for your use case.

I'm also wondering why you chose to use to Mongoid? I don't think a document store is a good data storage here? I think a key/value store like Redis would be a better alternative here because you basically just do key lookup and you can store everything in a hash. But again, I don't know the requirements.