Skip to content

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.

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.

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.

3 participants