Refactor #1
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| AllCops: | ||
| Exclude: | ||
| - 'bin/*' | ||
| - 'config/**/*' | ||
| - 'db/**/*' | ||
| - 'vendor/bundle/**/*' | ||
| - '**/Gemfile' | ||
| - '**/Rakefile' | ||
| - 'spec/spec_helper.rb' | ||
| - 'spec/rails_helper.rb' | ||
| - 'spec/support/**/*' | ||
| - 'node_modules/**/*' | ||
| - 'app/views/**/*' | ||
|
|
||
| Rails: | ||
| Enabled: true | ||
|
|
||
| Naming/VariableNumber: | ||
| EnforcedStyle: 'snake_case' | ||
|
|
||
| Layout/SpaceInsideHashLiteralBraces: | ||
| EnforcedStyle: no_space | ||
|
|
||
| Style/Documentation: | ||
| Enabled: false | ||
|
|
||
| Metrics/BlockLength: | ||
| Exclude: | ||
| - 'spec/**/*.rb' | ||
|
|
||
| Metrics/LineLength: | ||
| Max: 105 | ||
|
|
||
| Metrics/MethodLength: | ||
| Max: 20 | ||
|
|
||
| Style/Lambda: | ||
| EnforcedStyle: literal |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,28 +5,20 @@ def new | |
| end | ||
|
|
||
| def create | ||
| @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 |
||
|
|
||
| if @url.save | ||
| render js: "document.getElementById('short_url_box').classList | ||
| .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. |
||
| .innerText='http://#{request.host}/#{@url.url_short}/';" | ||
| else | ||
| flash[:alert] = @url.errors.full_messages | ||
| redirect_to :root | ||
| end | ||
| end | ||
|
|
||
| def show | ||
| @url = Url.find_by(url_short: params[:url_short]) | ||
| redirect_to "http://#{@url.url_clean}/" | ||
|
There was a problem hiding this comment. What if |
||
| end | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| module LinksGenerator | ||
| extend ActiveSupport::Concern | ||
|
There was a problem hiding this comment. I'd rather use Ruby |
||
|
|
||
| def new_link? | ||
| find_url_clean_duplicate.nil? | ||
| end | ||
|
|
||
| def find_url_clean_duplicate | ||
| Url.find_by(url_clean: url_clean) | ||
| end | ||
|
|
||
| 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 |
||
| clean_url.slice!(-1) if clean_url[-1] == '/' | ||
| self.url_clean = clean_url | ||
|
There was a problem hiding this comment. Concerns and modules that rely on the presence of some attributes or external methods indicate that those aren't the appropriate abstractions for this problem. Ideally concerns / modules should be independent on the class they are included in, otherwise it's just a fancy way to split the code between different files |
||
| end | ||
|
|
||
| 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.
|
||
| self.url_short = short_url | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| class CreateShortUrlService | ||
| def initialize(params) | ||
|
There was a problem hiding this comment. Consider using |
||
| @url_orginal = params[:url_orginal] | ||
|
There was a problem hiding this comment. Consider using private |
||
| end | ||
|
|
||
| def call | ||
| @url = Url.new(url_orginal: @url_orginal) | ||
| @url.clean_url | ||
| if @url.new_link? | ||
| @url | ||
| else | ||
| Url.find_by(url_clean: @url.url_clean) | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,14 @@ | ||
| class UrlValidator < ActiveModel::EachValidator | ||
| 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( |
||
| end | ||
|
|
||
| def url_valid?(url) | ||
| begin | ||
| url = URI.parse(url) | ||
| rescue URI::InvalidURIError | ||
| false | ||
| end | ||
| url.is_a?(URI::HTTP) || url.is_a?(URI::HTTPS) | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| require 'rails_helper' | ||
|
|
||
| RSpec.describe UrlsController do | ||
|
There was a problem hiding this comment. As far as I remember there's no need to use |
||
| describe 'POST create' do | ||
| let(:valid_params) { {'url' => {'url_orginal' => 'http://wkostanski.pl'}} } | ||
|
There was a problem hiding this comment. Consider using |
||
| let(:invalid_params) { {'url' => {'url_orginal' => 'wkostanski.pl'}} } | ||
| it 'with valid params' do | ||
|
There was a problem hiding this comment. Empty line before each |
||
| expect do | ||
| post :create, params: valid_params | ||
|
There was a problem hiding this comment. This line is duplicated across 3 tests therefore you can reduce the duplication by moving it to a subject, e.g subject { post :create, params: valid_params }
it 'something' do
expect(subject).to change(....)
end |
||
| end.to change(Url, :count).by(1) | ||
| end | ||
| it 'with invalid params' do | ||
| expect do | ||
| post :create, params: invalid_params | ||
| end.to change(Url, :count).by(0) | ||
| end | ||
| it 'with duplicated params' do | ||
| post :create, params: valid_params | ||
| expect do | ||
| post :create, params: valid_params | ||
| end.to change(Url, :count).by(0) | ||
| end | ||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
It'd be nice to move it to
development, :testgroup though