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

work on satisfactory reviews #5

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

Conversation

Fallonious
Copy link

No description provided.

@employees = []
end

def department_name
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. I'm glad that you know you need these methods. You can also simplify your code by writing attr_reader :department_name and attr_reader :employees to have Ruby generate these methods for you automatically.

@masonfmatthews
Copy link
Contributor

Fallon -

I like your tests; you've written them well. You also have clean code that doesn't go down too many wrong paths.

Don't forget the following:

  • Commit messages need to be present tense and singular (e.g. Create Employee class)
  • You don't need to create instances of your classes in the class files.

My big concern is how far you made it. You never got around to giving departmental raises, which was a requirement for this assignment. This could become an issue as we progress into next week and beyond, so let's have a chat when you get a chance to read this and digest.

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