Conversation
wojkos
commented
Nov 3, 2018
wojkos
commented
- adding rubocop
- adding tests
- fix typo and styles by rubocop
- move url model methods to concern
- created service for creating url
| gem 'haml', '~> 5.0', '>= 5.0.4' | ||
| gem 'bootstrap', '~> 4.1.3' | ||
| gem 'jquery-rails' | ||
| gem 'rubocop', require: false |
There was a problem hiding this comment.
👍
It'd be nice to move it to development, :test group though
| render js: "document.getElementById('short_url_box').classList | ||
| .remove('d-none');document.getElementById('short_url') | ||
| .innerText='http://#{request.host}/#{@url_in_database.url_short}/';" | ||
| .remove('d-none');document.getElementById('short_url') |
There was a problem hiding this comment.
Rendering of JS code from controllers is considered to be a bad practice since essentially you are mixing responsibilities and it makes the code harder to maintain, extend and reason about.
https://reinteractive.com/posts/195-rails-server-sent-javascript-considered-harmful
| def create | ||
| @url = Url.new(url_params) | ||
| @url.clean_url | ||
| @url = CreateShortUrlService.new(url_params).call |
There was a problem hiding this comment.
It's a matter of preference but I'd consider the usage of Callable module / class so that this line could be rewritten like CreateShortUrlService.call(url_params)
| def show | ||
| @url = Url.find_by_url_short(params[:url_short]) | ||
| @url = Url.find_by(url_short: params[:url_short]) | ||
| redirect_to "http://#{@url.url_clean}/" |
There was a problem hiding this comment.
What if @url is nil?
You can either use find_by!(notice the !) in combination with rescue_from ActiveRecord::RecordNotFound or manually handle it by checking whether the @url is present beforehand
|
|
||
| def clean_url | ||
| clean_url = url_orginal.strip | ||
| clean_url = clean_url.downcase.gsub(%r{(https?:\/\/)|(www\.)}, '') |
There was a problem hiding this comment.
It's advisable to move regexes to the constant
| def validate_each(record, attribute, value) | ||
| record.errors[attribute] << (options[:message] || "must be a valid URL") unless url_valid?(value) | ||
| def validate_each(record, attribute, value) | ||
| record.errors[attribute] << (options[:message] || 'must be a valid URL') unless url_valid?(value) |
There was a problem hiding this comment.
It's considered a good practice to move static strings to a constants. Also - don't forget to freeze it(freeze or #frozen_string_literal: true at the top of the file) to prevent unnecessary memory allocation
| @@ -0,0 +1,15 @@ | |||
| class CreateShortUrlService | |||
| def initialize(params) | |||
| @url_orginal = params[:url_orginal] | |||
There was a problem hiding this comment.
Consider using private attr_reader for encapsulation & increased flexibility
| validates :url_orginal, presence: true | ||
| validates :url_orginal, url: true | ||
|
|
||
| before_create :generate_short_url |
There was a problem hiding this comment.
This is an indication of a bad architecture since you're using a method defined in a module as a callback on the model therefore it's easily breakable by excluding the module
|
|
||
| def generate_short_url | ||
| short_url = generate_string_url(8) | ||
| short_url = generate_string_url(8) until Url.find_by(url_short: generate_string_url(8)).nil? |
There was a problem hiding this comment.
while Url.exists?(url_short: ...) should be faster
| @@ -0,0 +1,24 @@ | |||
| module LinksGenerator | |||
| extend ActiveSupport::Concern | |||
There was a problem hiding this comment.
I'd rather use Ruby self.included, self.extended hooks as opposed to Concerns but that's debatable so feel free to read on the topic and decide for yourself 🙂
2 participants
