-
-
Save camillebaldock/ef954f8e65262c5feaa7 to your computer and use it in GitHub Desktop.
class Employee | |
attr_reader :name, :month_salary | |
def initialize(args) | |
@name = args[:name] | |
@month_salary = args[:month_salary] | |
end | |
def vat | |
0.20 | |
end | |
def gross_salary_pa | |
month_salary * 12 | |
end | |
def net_salary_pa | |
gross_salary_pa - gross_salary_pa * vat | |
end | |
end |
require "./app/employee.rb" | |
require "test/unit" | |
class EmployeeTest < Test::Unit::TestCase | |
def test_instantiation | |
employee = Employee.new(name: "Jack", month_salary: 1000) | |
assert_equal(Employee, employee.class) | |
assert_equal("Jack", employee.name) | |
assert_equal(1000, employee.month_salary) | |
assert_equal(12000, employee.gross_salary_pa) | |
assert_equal(9600, employee.net_salary_pa) | |
end | |
end |
require "./app/employee.rb" | |
class PayrollEmployee < Employee | |
def gross_salary_pa | |
month_salary * 13 | |
end | |
end |
require "./app/payroll_employee.rb" | |
require "test/unit" | |
class PayrollEmployeeTest < Test::Unit::TestCase | |
def payroll_emp | |
payroll_emp = PayrollEmployee.new(name: "Jack", month_salary: 1000) | |
end | |
def test_intantiation | |
assert_equal("Jack", payroll_emp.name) | |
assert_equal(1000, payroll_emp.month_salary) | |
end | |
def test_salary | |
assert_equal(13000, payroll_emp.gross_salary_pa) | |
assert_equal(10400, payroll_emp.net_salary_pa) | |
end | |
end |
require "./app/sales_employee.rb" | |
require "test/unit" | |
class SalesEmployeeTest < Test::Unit::TestCase | |
def test_intantiation | |
sales_emp = SalesEmployee.new(name: "Jack", month_salary: 1000, sales: 10000) | |
assert_equal(SalesEmployee, sales_emp.class) | |
assert_equal("Jack", sales_emp.name) | |
assert_equal(1000, sales_emp.month_salary) | |
assert_equal(10000, sales_emp.sales) | |
assert_equal(2000, sales_emp.bonus) | |
assert_equal(14000, sales_emp.gross_salary_pa) | |
assert_equal(11200, sales_emp.net_salary_pa) | |
end | |
end |
require "./app/payroll_employee.rb" | |
require "./app/sales_employee.rb" | |
class System | |
attr_reader :employees | |
def initialize(args) | |
@employees = args[:employees] | |
end | |
def emp_names | |
list = [] | |
employees.each do |employee| | |
names(list,employee) | |
end | |
list | |
end | |
def tot_gross_salary_pa | |
total = 0 | |
employees.each do |employee| | |
total += emp_gross_salary_pa(employee) | |
end | |
total | |
end | |
private | |
def emp_name(employee) | |
employee.name | |
end | |
def names(list,employee) | |
list << emp_name(employee) | |
end | |
def emp_gross_salary_pa(employee) | |
employee.gross_salary_pa | |
end | |
end |
require "./app/system.rb" | |
require "test/unit" | |
class SystemTest < Test::Unit::TestCase | |
def system | |
System.new(employees: [ | |
PayrollEmployee.new(name: "Jack", month_salary: 1000), | |
SalesEmployee.new(name: "Brad", month_salary: 1000, sales: 10000) | |
]) | |
end | |
def test_instantiation | |
assert_equal(System, system.class) | |
end | |
def test_emp_names | |
assert_equal(["Jack", "Brad"], system.emp_names) | |
end | |
def test_tot_salary | |
assert_equal(27000, system.tot_gross_salary_pa) | |
end | |
end |
Use of fetch vs []
The example I am going to take here is in employee.rb
def initialize(args)
@name = args[:name]
@month_salary = args[:month_salary]
end
This will work fine in the case that the args Hash has a name and month_salary. It will fail silently if it does not. In most situations, in particular initialisers, I would recommend you use args.fetch(:name)
instead of args[:name]
. This will mean that if there is no :name key/value pair in the args hash, it will throw an Exception. I wouldn't be super-happy to create an employee without error, and then when I for example call gross_salary_pa
on it and it fails then. I would rather it fails during creation.
Fetch also enables to set a default value, for example something along the lines of:
@month_salary = args.fetch(:month_salary, MINIMUM_MONTHLY_WAGE)
Public vs private
Without knowing more details about how you are planning on using this code, deciding what should be private and public is obviously very much up to you :-)
It feels to me however that a couple more methods could be private, eg the vat
method on Employee
. A couple of the instance variables that you make accessible via attr_reader
could also possibly be private.
Use of constants
For constant values like VAT, I would consider not using a method but rather using a constant and potentially freezing it? Again, very much up to how you are expecting to handle that value in your objects :-)
Test structure
There are two things that I believe you can improve on the structure and meaning of your tests.
- Structure
A commonly used structure for organising the content of your tests is Arrange/Act/Assert. Most testing frameworks force you to organise your tests in that structure, and you are mostly doing so. The part that would make your tests more readable and descriptive is:
- if your tests were divided up more granularly
- if they were described in a more meaningful way
Enough waffle, some examples :-)
In the case of your SalesEmployee test you are actually testing a lot more than instantiation
def test_intantiation
sales_emp = SalesEmployee.new(name: "Jack", month_salary: 1000, sales: 10000)
assert_equal(SalesEmployee, sales_emp.class)
assert_equal("Jack", sales_emp.name)
assert_equal(1000, sales_emp.month_salary)
assert_equal(10000, sales_emp.sales)
assert_equal(2000, sales_emp.bonus)
assert_equal(14000, sales_emp.gross_salary_pa)
assert_equal(11200, sales_emp.net_salary_pa)
end
This is actually testing seven different methods :-) and would be more readable if separated out in two or three tests with descriptions :-)
eg the first 5 lines of your test are looking at instantiation and checking your instance variables have been set properly during initialization.
the other lines seem to be testing other things and therefore can be separated out for extra readability.
Some possible refactorings in system.rb
1
employees.each do |employee|
names(list,employee)
end
def names(list,employee)
list << emp_name(employee)
end
is actually a reimplementation of a map
.
employees.map do |employee|
emp_name(employee)
end
Actually since emp_name
is actually doing employee.name
, I'm not quite sure why you would want that emp_name
method there in the first place and therefore you could probably do:
employees.map do |employee|
employee.name
end
which actually can be "golfed" down to:
employees.map(&:name)
2
The same kind of refactoring could be applied to the tot_gross_salary_pa
method :-)
employees.map(&: gross_salary_pa)..reduce(:+)
Signing off now :-)
Thank you for sharing this with me!
My overall comment for this is well done!
I particularly liked that you took care of:
- writing carefully named methods
- thoroughly testing things
- thinking about your
Employee
vsSalesEmployee
structure of inheritance
As all code things, there are always aspects that can be changed and improved but this is a very solid solution to the exercise 👍
Wow! Thanks Camille, that's more than expected!
I will study all you said properly and get back to you, you're the best! 😄
Many thanks again.
Jack
Commenting on this Github gist from the Github office: the meta quality of this is not lost on me :-)
Leaving comments on this gist as slightly easier than bulk-commeting on email. :-)
Non-life-changing general comments: