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

Wade branch #16

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Wade branch #16

wants to merge 8 commits into from

Conversation

gwadej
Copy link

@gwadej gwadej commented Oct 9, 2015

Two steps into this challenge, one commit per release.

Please review @chivygab @jaybobo @mikegee

@gwadej
Copy link
Author

gwadej commented Oct 9, 2015

Added two more commits for the other parts.

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

create_table "urls", force: true do |t|
t.string "linkid", limit: UrlsHelper::LINKID_LENGTH, null: false
Copy link
Member

Choose a reason for hiding this comment

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

Was this created by a migration? If not, please read the comment at the top of this file for some guidance.

Copy link
Author

Choose a reason for hiding this comment

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

I normally don't use code generators/wizards until I understand them. I spent a fair amount of time working to understand this and then did not go back to the migrations.

I will correct that in a new pull request.

G. Wade Johnson added 3 commits October 12, 2015 08:18
Instead of creating by hand, I used rails generate to build the initial
model.
After a challenge from Mike on the last version, I tried to do this
without building a whole, separate validation class.

I couldn't get message to work on the 'presence' validation, so I did a
separate validation function for it. I could have combined all of these
into one large validation function, but I decided it was more
interesting to make a number of simple validation functions instead.
@gwadej
Copy link
Author

gwadej commented Oct 13, 2015

This branch has been completely re-built using migrations this time.
I've also incorporated some changes the @mikegee hinted at, that have improved the code quite a bit.

Please review @chivygab @jaybobo @mikegee

@gwadej
Copy link
Author

gwadej commented Oct 14, 2015

Filled out the tests for the model and controllers. Still trying to get used to using the generator.

@errors = nil

@url = Url.new(target_link: @target)
if [email protected]
Copy link
Member

Choose a reason for hiding this comment

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

I prefer the happy path as the positive part of the if and the error path under else.

Copy link
Author

Choose a reason for hiding this comment

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

Fair enough.

* if/else handling of error
* Use of .blank?
* Correct start of string anchor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants