Skip to content

Homework#9

Open
owenrham wants to merge 10 commits intotiyd-rails-2015-01:masterfrom
owenrham:master
Open

Homework#9
owenrham wants to merge 10 commits intotiyd-rails-2015-01:masterfrom
owenrham:master

Conversation

@owenrham
Copy link

No description provided.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great parameter variable name.

On the flipside, I'd suggest using full words in variable and method names, though. "department_raise" is better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, you could just call it "raise", as it's already in the context of a department.

@masonfmatthews
Copy link
Contributor

Owen -

Your code is good, and your testing strategy is correct as well. I think you write a bit more code than you need to, and your concepts are stomping on each other a bit (e.g. you stick a bit more under attr_accessor than makes sense), but you are certainly getting the job done. I think the nuances will continue to accumulate for you over time.

If you have any concerns that you'd like to discuss, just let me know. Otherwise, I think that you're right on track for the class.

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