Skip to content
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

Levi's solution #35

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions source/Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ gem 'coffee-rails', '~> 4.0.0'

# Use jquery as the JavaScript library
gem 'jquery-rails'
# Turbolinks makes following links in your web application faster. Read more: https://github.com/rails/turbolinks
gem 'turbolinks'
# Build JSON APIs with ease. Read more: https://github.com/rails/jbuilder
gem 'jbuilder', '~> 2.0'
# bundle exec rake doc:rails generates the API under doc/api.
Expand All @@ -38,4 +36,3 @@ gem 'spring', group: :development
# Use debugger
# gem 'debugger', group: [:development, :test]
gem 'rspec-rails', group: [:development, :test]

3 changes: 0 additions & 3 deletions source/Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,6 @@ GEM
thor (0.19.1)
thread_safe (0.3.4)
tilt (1.4.1)
turbolinks (2.4.0)
coffee-rails
tzinfo (1.2.2)
thread_safe (~> 0.1)
uglifier (2.5.3)
Expand All @@ -134,5 +132,4 @@ DEPENDENCIES
sdoc (~> 0.4.0)
spring
sqlite3
turbolinks
uglifier (>= 1.3.0)
1 change: 0 additions & 1 deletion source/app/assets/javascripts/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,4 @@
//
//= require jquery
//= require jquery_ujs
//= require turbolinks
//= require_tree .
35 changes: 35 additions & 0 deletions source/app/controllers/urls_controller.rb
Original file line number Diff line number Diff line change
@@ -1,2 +1,37 @@
class UrlsController < ApplicationController

# GET /urls
def index
@urls = Url.all
@url = Url.new
end

# POST /urls
def create
@url = Url.new(url_params)

respond_to do |format|
if @url.save
format.html { redirect_to root_path, notice: 'Url was successfully created.' }
else
format.html { redirect_to root_path, notice: @url.errors.full_messages }
end
end
end

# GET /:shortened_url
def redirect
@url = Url.where(shortened_url: params[:shortened_url]).take
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

take with no argument seems very out of place to me. I think most Rails devs would expect it's alias, first.

find_by simplifies this a bit more though.

if @url != nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ruby provides nil? on every object for just this sort of check.

But, since you're checking for "not nil" the Rails method present? would do the trick.

Even better (in my opinion) is to drop the nil or presence check and depend of the fact that nil is falsey. Just: if @url.

@url.update_attribute(:click_count, @url.click_count + 1)
redirect_to @url.original_url
else
redirect_to root_path, notice: 'We couldn\'t find a link for the bitly URL you clicked.'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching to double quotes is preferable over escaping the embedded single quote.

end
end

private
def url_params
params.require(:url).permit(:original_url, :shortened_url, :click_count)
end
end
41 changes: 41 additions & 0 deletions source/app/models/url.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
require 'uri'
require 'net/http'

class Url < ActiveRecord::Base
ALPHANUMERIC_CHARACTERS = (('a'..'z').to_a + ('A'..'Z').to_a + ('0'..'9').to_a)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 extracting a constant for this


validates_presence_of :original_url
validates_format_of :original_url, :with => URI::regexp, :message => "must be a valid Ruby URI"
validate :original_url_starts_with_http_or_https
validate :original_url_must_return_a_response

before_save :shorten_url

def original_url_starts_with_http_or_https
if not original_url =~ /^https?:\/\//
errors.add(:original_url, "must start with http:// or https://")
end
end

def original_url_must_return_a_response
return unless original_url.present? && original_url =~ URI::regexp(['http', 'https'])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Matching the url against URI's regexp helper gizmo is somewhat duplicative with your check in original_url_starts_with_http_or_https. I wonder how we can remove the duplicated concept. 🤔

uri = URI.parse(original_url)
response = Net::HTTP.get_response(uri)
rescue URI::InvalidURIError
errors.add(:original_url, "must respond to a HTTP request")
rescue Errno::ECONNREFUSED
errors.add(:original_url, "must respond to a HTTP request")
rescue SocketError
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect Net::HTTP is capable of raise many other errors. I'd switch to rescuing all subclasses of StandardError with bare rescue and make sure only Net::HTTP code is invoked in this method. (just the URI.parse and the get_response parts)

errors.add(:original_url, "must respond to a HTTP request")
end

def shorten_url
self.shortened_url = generate_key unless self.shortened_url
end

def generate_key
key = ""
7.times { key << ALPHANUMERIC_CHARACTERS.sample }
key
end
end
4 changes: 2 additions & 2 deletions source/app/views/layouts/application.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
<html>
<head>
<title>Source</title>
<%= stylesheet_link_tag 'application', media: 'all', 'data-turbolinks-track' => true %>
<%= javascript_include_tag 'application', 'data-turbolinks-track' => true %>
<%= stylesheet_link_tag 'application', media: 'all' %>
<%= javascript_include_tag 'application' %>
<%= csrf_meta_tags %>
</head>
<body>
Expand Down
19 changes: 19 additions & 0 deletions source/app/views/urls/_form.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<%= form_for(@url) do |f| %>
<% if @url.errors.any? %>
<div id="error_explanation">
<h2><%= pluralize(@url.errors.count, "error") %> prohibited this url from being saved:</h2>

<ul>
<% @url.errors.full_messages.each do |message| %>
<li><%= message %></li>
<% end %>
</ul>
</div>
<% end %>

<div class="field">
<%= f.text_field :original_url %>
<%= f.hidden_field :click_count, value: 0 %>
<%= f.submit 'Shorten' %>
</div>
<% end %>
27 changes: 27 additions & 0 deletions source/app/views/urls/index.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<h2>Shorten a link</h2>

<%= render 'form' %>

<br><hr>

<p id="notice"><%= notice %></p>

<h2>Your shortened links</h2>

<table>
<tbody>
<% @urls.each do |url| %>
<tr>
<td><%= url.original_url %></td>
</tr>
<tr>
<td><%= link_to "http://localhost:3000/#{url.shortened_url}", "http://localhost:3000/#{url.shortened_url}" %></td>
<td>&nbsp;&nbsp;&nbsp;</td>
<td><b>Clicks:</b> <%= url.click_count %></td>
</tr>
<tr>
<td>&nbsp;</td>
</tr>
<% end %>
</tbody>
</table>
57 changes: 3 additions & 54 deletions source/config/routes.rb
Original file line number Diff line number Diff line change
@@ -1,56 +1,5 @@
Rails.application.routes.draw do
# The priority is based upon order of creation: first created -> highest priority.
# See how all your routes lay out with "rake routes".

# You can have the root of your site routed with "root"
# root 'welcome#index'

# Example of regular route:
# get 'products/:id' => 'catalog#view'

# Example of named route that can be invoked with purchase_url(id: product.id)
# get 'products/:id/purchase' => 'catalog#purchase', as: :purchase

# Example resource route (maps HTTP verbs to controller actions automatically):
# resources :products

# Example resource route with options:
# resources :products do
# member do
# get 'short'
# post 'toggle'
# end
#
# collection do
# get 'sold'
# end
# end

# Example resource route with sub-resources:
# resources :products do
# resources :comments, :sales
# resource :seller
# end

# Example resource route with more complex sub-resources:
# resources :products do
# resources :comments
# resources :sales do
# get 'recent', on: :collection
# end
# end

# Example resource route with concerns:
# concern :toggleable do
# post 'toggle'
# end
# resources :posts, concerns: :toggleable
# resources :photos, concerns: :toggleable

# Example resource route within a namespace:
# namespace :admin do
# # Directs /admin/products/* to Admin::ProductsController
# # (app/controllers/admin/products_controller.rb)
# resources :products
# end
resources :urls, only: [:index, :create]
get '/:shortened_url', to: 'urls#redirect'
root 'urls#index'
end
10 changes: 10 additions & 0 deletions source/db/migrate/20180125164843_create_urls.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
class CreateUrls < ActiveRecord::Migration
def change
create_table :urls do |t|
t.string :original_url
t.string :shortened_url

t.timestamps
end
end
end
5 changes: 5 additions & 0 deletions source/db/migrate/20180125194335_add_click_count_to_urls.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddClickCountToUrls < ActiveRecord::Migration
def change
add_column :urls, :click_count, :integer
end
end
24 changes: 24 additions & 0 deletions source/db/schema.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# encoding: UTF-8
# This file is auto-generated from the current state of the database. Instead
# of editing this file, please use the migrations feature of Active Record to
# incrementally modify your database, and then regenerate this schema definition.
#
# Note that this schema.rb definition is the authoritative source for your
# database schema. If you need to create the application database on another
# system, you should be using db:schema:load, not running all the migrations
# from scratch. The latter is a flawed and unsustainable approach (the more migrations
# you'll amass, the slower it'll run and the greater likelihood for issues).
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 20180125194335) do

create_table "urls", force: true do |t|
t.string "original_url"
t.string "shortened_url"
t.datetime "created_at"
t.datetime "updated_at"
t.integer "click_count"
end

end