Refactor #1
Refactor #1
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
| @@ -17,6 +17,7 @@ gem 'jbuilder', '~> 2.5' | |||
| 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
| @@ -5,28 +5,20 @@ def new | |||
| end | |||
|
|
|||
| 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)
| end | ||
| end | ||
|
|
||
| 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
| @@ -1,31 +1,9 @@ | |||
| class Url < ApplicationRecord | |||
| include CharsArray | |||
| include LinksGenerator | |||
|
|
|||
| 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 🙂
