2
\$\begingroup\$

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.

\$\endgroup\$
0

1 Answer 1

2
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ Thanks! What are the implications of error handling with each of these approaches? I'm partial to the fat model. Cheers! \$\endgroup\$
    – McDerp
    Oct 4, 2020 at 23:22
  • \$\begingroup\$ That really depends which kind of errors you would expect? You could add errors to your model (docs.mongodb.com/mongoid/7.1/tutorials/mongoid-validation/…), throw an exception, return a result object. There are several different way but it depends really on your use case. Can you give some examples? \$\endgroup\$ Oct 5, 2020 at 21:01
  • \$\begingroup\$ Let me rephrase... this is for a sort of a quiz. Say you were looking at a proof of concept applet that did this, what would you expect to see to tick the box "This guy has thought about idiomatic error handling in this demo app"? I know how to handle errors, just rusty with ruby. What does an app with two routes look like in terms of idiomatic erro handling? I imagine there should be some safeguards like "db not available" or "malformed incoming url" (helper for that to be added I suppose) \$\endgroup\$
    – McDerp
    Oct 5, 2020 at 21:33
  • \$\begingroup\$ Added some more comments to the original answer. \$\endgroup\$ Oct 8, 2020 at 22:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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