Conversation
jaybobo
left a comment
jaybobo
left a comment
There was a problem hiding this comment.
Looks good. Check out my comments
source/app/models/url.rb
Outdated
| validate :url_is_correct_format, :url_responds_to_http_request | ||
|
|
||
| def url_is_correct_format | ||
| unless address =~ /\A#{URI.regexp}\z/ |
| require 'rails_helper' | ||
|
|
||
| RSpec.describe Url, :type => :model do | ||
| pending "add some examples to (or delete) #{__FILE__}" |
There was a problem hiding this comment.
how would you test the feature that you wrote?
| end | ||
| end | ||
|
|
||
| def generate_shortened_url |
There was a problem hiding this comment.
In most cases, we'd use a random string generator with the length we desire instead of mutating the original address. It's cleaner, easier & more secure.
There was a problem hiding this comment.
| end | ||
|
|
||
| def url_responds_to_http_request | ||
| url = URI.parse(address) |
There was a problem hiding this comment.
Could a bad actor use this logic to DOS another site possibly?
There was a problem hiding this comment.
@jaybobo Yeah, they could. Are there any specific safe practices I could look up? Most of my generic searches just send me to security companies.
| @url.errors.full_messages | ||
| @url.errors.each { |error| puts error.full_messages } | ||
| @url.errors.each do |error| | ||
| @url.errors |
There was a problem hiding this comment.
This file should probably go in .gitignore - no reason for it to be part of the history of the repo.
source/app/models/url.rb
Outdated
|
|
||
| def generate_shortened_url | ||
| url = URI.parse(address) | ||
| if url.host.present? |
There was a problem hiding this comment.
If you can move this check for host presence into a separate validator, then you can avoid having all of this code be nested inside an if-block.
There was a problem hiding this comment.
I redid the method per Jay's recommendation of random string generation and let the validators worry about URL validity
source/app/models/url.rb
Outdated
| while !self.slug && Url.find_by(slug: temp_shortened_host) | ||
| count += 1 | ||
| temp_shortened_host = "#{shortened_host}#{count}" | ||
| end |
There was a problem hiding this comment.
This is going to generate a database query for every pass through the loop. Why not use a LIKE query to grab all of the relevant items, and then find the one with the maximum counter on the end from there?
There was a problem hiding this comment.
Good idea! I changed to random string generation and thus made like strings redundant. I did add an index to speed up the comparison and routing
| # # (app/controllers/admin/products_controller.rb) | ||
| # resources :products | ||
| # end | ||
| get '/:slug', to: 'urls#show' |
|
|
||
| def create | ||
| @url = Url.new(url_params) | ||
| @url.click_count = 0 |
There was a problem hiding this comment.
If you put a default value in the database, ActiveRecord will pick it up and you won't need this line.
| def show | ||
| @url = Url.find_by(slug: params[:slug]) | ||
| @url.click_count += 1 | ||
| @url.save |
There was a problem hiding this comment.
ActiveRecord has a method for this sort of thing.
There's a subtle bug as currently written. Do you see it?
There was a problem hiding this comment.
Was it that there was no error handling for invalid slugs?
There was a problem hiding this comment.
No, concurrent requests for the same slug would undercount the clicks.
There was a problem hiding this comment.
Would you recommend locking? Or is there a better way to do this?
There was a problem hiding this comment.
Oops. I thought the increment method did an atomic increment in the database. Looking at its implementation now, I see that it does the same thing your code did here: load the value into memory, add one, then save the new value.
A lock would certainly resolve this (and ActiveRecord has a with_lock method to make it easy), but I prefer a SQL update command without a lock.
There was a problem hiding this comment.
I discovered the class method :increment_counter which does an atomic increment of the field given the id and have implemented it in my latest commit. Thanks for showing me this as I now have some understanding of concurrency issues and atomicity
4 participants
@jaybobo