-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor #1
base: master
Are you sure you want to change the base?
Refactor #1
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
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 = Url.new(url_params) | ||
@url.clean_url | ||
@url = CreateShortUrlService.new(url_params).call | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a matter of preference but I'd consider the usage of |
||
|
||
if @url.new_link? | ||
if @url.save | ||
render js: "document.getElementById('short_url_box').classList | ||
.remove('d-none');document.getElementById('short_url') | ||
.innerText='http://#{request.host}/#{@url.url_short}/';" | ||
else | ||
flash[:alert] = @url.errors.full_messages | ||
redirect_to :root | ||
end | ||
else | ||
@url_in_database = Url.find_by_url_clean(@url.url_clean) | ||
if @url.save | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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]) | ||
@url = Url.find_by(url_short: params[:url_short]) | ||
redirect_to "http://#{@url.url_clean}/" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 new_link? | ||
find_duplicate.nil? | ||
end | ||
|
||
def find_duplicate | ||
Url.find_by_url_clean(url_clean) | ||
end | ||
|
||
def clean_url | ||
clean_url = url_orginal.strip | ||
clean_url = clean_url.downcase.gsub(/(https?:\/\/)|(www\.)/, '') | ||
clean_url.slice!(-1) if clean_url[-1] == '/' | ||
self.url_clean = clean_url | ||
end | ||
|
||
def generate_short_url | ||
short_url = generate_string_url(8) | ||
until Url.find_by_url_short(generate_string_url(8)).nil? | ||
short_url = generate_string_url(8) | ||
end | ||
self.url_short = short_url | ||
end | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using |
||
@url_orginal = params[:url_orginal] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 | ||
|
||
def url_valid?(url) | ||
url = URI.parse(url) rescue false | ||
url.kind_of?(URI::HTTP) || url.kind_of?(URI::HTTPS) | ||
end | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using |
||
let(:invalid_params) { {'url' => {'url_orginal' => 'wkostanski.pl'}} } | ||
it 'with valid params' do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Empty line before each |
||
expect do | ||
post :create, params: valid_params | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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, :test
group though