PayCheck calculator software

Question

I was participating in a job selection process and one of the tasks I was asked to do was to develop a take home pay application to let users to calculate their paychecks based on their location. Today I received the feedback, and it stated that:

• Taxes poorly factored
• Low OO knowledge

The software requirements are stated below:

Write a program which can be used to calculate the take home pay for employees based on their location. The application should be a console app which prompts the user for the hourly rate and hours worked for the employee.

Please enter the hours worked: Please enter the hourly rate: Please enter the employee’s location: The program will output their gross along with a list of deductions along with a net amount.

Employee location: Ireland

Gross Amount: €x

Less deductions

Income Tax : €x Universal Social Charge: €x Pension: €x Net Amount: €x

Requirements

As a payroll user I would like to see a gross amount calculation for an employee’s salary.

Given the employee is paid 10ドル per hour, when the employee works 40 hours, the Gross amount is 400ドル

As a payroll user I would like to see deductions charged for employees located in Ireland

Given the employee is located in Ireland, income tax at a rate of 25% for the first 600ドル and 40% thereafter Given the employee is located in Ireland, a Universal social charge of 7% is applied for the first 500ドル euro and 8% thereafter Given the employee is located in Ireland, a compulsory pension contribution of 4% is applied

As a payroll user I would like to see deductions charged for employees located in Italy

Given the employee is located in Italy, income tax at a flat rate of 25% is applied Given the employee is located in Italy, an INPS contribution of is applied based on gross salary. This is charged at 9% for the first 500ドル and increases by .1% over every 100ドル thereafter.

As a payroll user I would like to see deductions charged for employees located in Germany

Given the employee is located in Germany, income tax at a rate of 25% is applied on the first 400ドル and 32% thereafter Given the employee is located in Germany, a compulsory pension contribution of 2% is applied

I really cannot understand why my code was so badly evaluated, so it would be wonderful to have some of you checking out what could have went so wrong.

Just to make it clear, I would like to have some of you guys checking out the code to try to find out some design flaws that could have motivated the bad evaluation I received from the tech team.

The code I wrote can be found, along with the requirements, here.

Here follows some important parts of the code:

Base class PayCheckCalculator

using Domain.Entities;
namespace Domain.Interface
{
 public abstract class PayCheckCalculator
 {
 protected void CalculateGrossAmount(PayCheck payCheck, int hoursWorked, decimal hourlyRate)
 {
 payCheck.GrossSalary = hoursWorked * hourlyRate;
 }
 protected abstract void CalculateDeductions(PayCheck payCheck);
 protected void CalculateNetAmount(PayCheck payCheck)
 {
 decimal netAmount = payCheck.GrossSalary;
 foreach (var deduction in payCheck.Deductions)
 {
 netAmount -= deduction.Amount;
 }
 payCheck.NetSalary = netAmount;
 }
 public PayCheck Calculate(int hoursWorked, decimal hourlyRate)
 {
 PayCheck payCheck = new PayCheck();
 CalculateGrossAmount(payCheck, hoursWorked, hourlyRate);
 CalculateDeductions(payCheck);
 CalculateNetAmount(payCheck);
 return payCheck;
 }
 }
}

Concrete class GermanyPayCheckCalculator

using Domain.Entities;
using Domain.Interface;
namespace PayCheckCalculators.Calculators
{
 public class GermanyPayCheckCalculator: PayCheckCalculator
 {
 protected override void CalculateDeductions(PayCheck payCheck)
 {
 payCheck.Deductions.Add(CalculateIncomeDeduction(payCheck));
 payCheck.Deductions.Add(CalculateCompulsoryPensionDeduction(payCheck));
 }
 private Deduction CalculateIncomeDeduction(PayCheck payCheck)
 {
 string deductionDesc = "Income tax";
 decimal amount = 0.0m;
 decimal surplus = payCheck.GrossSalary - 400;
 if (surplus <= 0)
 {
 amount = payCheck.GrossSalary * 0.25m;
 }
 else
 {
 amount = 400m * 0.25m;
 amount += surplus * 0.32m;
 }
 return new Deduction(deductionDesc, amount, Deduction.DeductionType.INCOME_DEDUCTION);
 }
 private Deduction CalculateCompulsoryPensionDeduction(PayCheck payCheck)
 {
 string deductionDesc = "Pension";
 return new Deduction(deductionDesc, payCheck.GrossSalary * 0.02m, Deduction.DeductionType.PENSION_DEDUCTION);
 }
 }
}

Concrete class IrelandPayCheckCalculator

using Domain.Interface;
using Domain.Entities;
namespace PayCheckCalculators.Calculators
{
 public class IrelandPayCheckCalculator : PayCheckCalculator
 {
 protected override void CalculateDeductions(PayCheck payCheck)
 {
 payCheck.Deductions.Add(CalculateIncomeDeduction(payCheck));
 payCheck.Deductions.Add(CalculateUniversalSocialChargeDeduction(payCheck));
 payCheck.Deductions.Add(CalculateCompulsoryPensionDeduction(payCheck));
 }
 private Deduction CalculateIncomeDeduction(PayCheck payCheck)
 {
 string deductionDesc = "Income tax";
 decimal amount = 0.0m;
 decimal surplus = payCheck.GrossSalary - 600;
 if (surplus <= 0)
 {
 amount = payCheck.GrossSalary * 0.25m;
 }
 else
 {
 amount = 600m * 0.25m;
 amount += surplus * 0.4m;
 }
 return new Deduction(deductionDesc, amount, Deduction.DeductionType.INCOME_DEDUCTION);
 }
 private Deduction CalculateUniversalSocialChargeDeduction(PayCheck payCheck)
 {
 string deductionDesc = "Universal social charge";
 decimal amount = 0.0m;
 decimal surplus = payCheck.GrossSalary - 500;
 if (surplus <= 0)
 {
 amount = payCheck.GrossSalary * 0.07m;
 }
 else
 {
 amount = 500m * 0.07m;
 amount += surplus * 0.08m;
 }
 return new Deduction(deductionDesc, amount, Deduction.DeductionType.UNIVERSAL_SOCIAL_DEDUCTION); ;
 }
 private Deduction CalculateCompulsoryPensionDeduction(PayCheck payCheck)
 {
 string deductionDesc = "Pension";
 return new Deduction(deductionDesc, payCheck.GrossSalary * 0.04m, Deduction.DeductionType.PENSION_DEDUCTION);
 }
 }
}

Concrete class ItalyPayCheckCalculator

using Domain.Interface;
using Domain.Entities;
namespace PayCheckCalculators.Calculators
{
 public class ItalyPayCheckCalculator : PayCheckCalculator
 {
 protected override void CalculateDeductions(PayCheck payCheck)
 {
 payCheck.Deductions.Add(CalculateIncomeDeduction(payCheck));
 payCheck.Deductions.Add(CalculateInpsDeduction(payCheck));
 }
 private Deduction CalculateIncomeDeduction(PayCheck payCheck)
 {
 string deductionDesc = "Income tax";
 return new Deduction(deductionDesc, payCheck.GrossSalary * 0.25m, Deduction.DeductionType.INCOME_DEDUCTION); ;
 }
 private Deduction CalculateInpsDeduction(PayCheck payCheck)
 {
 string deductionDesc = "INPS";
 decimal amount = 0.0m;
 decimal surplus = payCheck.GrossSalary - 500m;
 if (surplus <= 0)
 {
 amount = payCheck.GrossSalary * 0.09m;
 }
 else
 {
 amount = 500 * 0.09m;
 while (surplus >= 100)
 {
 amount += 1;
 surplus -= 100;
 }
 }
 return new Deduction(deductionDesc, amount, Deduction.DeductionType.INPS_DEDUCTION); ;
 }
 }
}

Abstract factory class

namespace Domain.Interface
{
 public abstract class AbstractPayCheckCalculatorFactory<T>
 {
 public abstract T CreatePayCheckCalculator(string location);
 }
}

Concrete factory class

using Domain.Interface;
using PayCheckCalculators.Calculators;
using System;
namespace PayCheckCalculators.Factory
{
 public class PayCheckCalculatorFactory: AbstractPayCheckCalculatorFactory<PayCheckCalculator>
 {
 public override PayCheckCalculator CreatePayCheckCalculator(string location)
 {
 PayCheckCalculator calculator;
 if (location == null)
 {
 throw new ArgumentNullException("Location cannot be null.");
 }
 if (location.Length == 0)
 {
 throw new ArgumentException("Location cannot be empty.");
 }
 switch (location.ToLower())
 {
 case "ireland": calculator = new IrelandPayCheckCalculator(); break;
 case "italy": calculator = new ItalyPayCheckCalculator(); break;
 case "germany": calculator = new GermanyPayCheckCalculator(); break;
 default: throw new NotSupportedException("The provided country is not yet supported.");
 }
 return calculator;
 }
 }
}

Employee class

using System;
using System.Collections.Generic;
using System.Linq;
using Domain.Validation;
namespace Domain.Entities
{
 public class Employee
 {
 public string Location { get; private set; }
 public int HoursWorked { get; private set; }
 public decimal HourlyRate { get; private set; }
 public Employee(string location, int hoursWorked = 0, decimal hourlyRate = 0.0m)
 {
 Location = location;
 HoursWorked = hoursWorked;
 HourlyRate = hourlyRate;
 var errors = Validate();
 if (errors.Count > 0)
 {
 var except = new ArgumentException();
 except.Data.Add("errors", errors);
 throw except;
 }
 }
 private ICollection<ValidationError> Validate()
 {
 var errors = new List<ValidationError>();
 errors.AddRange(ValidateHoursWorked());
 errors.AddRange(ValidateHourlyRate());
 errors.AddRange(ValidateLocation());
 return errors;
 }
 //TODO: I don't check for an upper limit for now because it must be discussed further with the PO. 
 private ICollection<ValidationError> ValidateHoursWorked()
 {
 var errors = new List<ValidationError>();
 if (HoursWorked < 0)
 {
 String description = "Error: Hours Worked cannot be negative.";
 errors.Add(new ValidationError(ValidationError.ErrorCode.IMPUT_OUT_OF_RANGE, description));
 }
 return errors;
 }
 //TODO: I don't check for an upper limit for now because it must be discussed further with the PO. 
 private ICollection<ValidationError> ValidateHourlyRate()
 {
 var errors = new List<ValidationError>();
 if (HourlyRate < 0)
 {
 String description = "Error: Hourly Rate cannot be negative.";
 errors.Add(new ValidationError(ValidationError.ErrorCode.IMPUT_OUT_OF_RANGE, description));
 }
 return errors;
 }
 private ICollection<ValidationError> ValidateLocation()
 {
 var errors = new List<ValidationError>();
 String description = "";
 if (Location == null)
 {
 description = "Error: Location cannot be null.";
 errors.Add(new ValidationError(ValidationError.ErrorCode.IMPUT_NULL, description));
 }
 else if (Location.Length == 0)
 {
 description = "Error: Location cannot be empty.";
 errors.Add(new ValidationError(ValidationError.ErrorCode.IMPUT_EMPTY, description));
 }
 else if (!Location.All(c => char.IsLetter(c)))
 {
 description = "Error: Location can only contain letters.";
 errors.Add(new ValidationError(ValidationError.ErrorCode.IMPUT_OUT_OF_FORMAT, description));
 }
 return errors;
 }
 }
}

Program class

using Domain.Entities;
using Domain.Interface;
using Domain.Validation;
using PayCheckApp.View;
using PayCheckCalculators.Factory;
using System;
using System.Collections.Generic;
namespace PayCheckApp
{
 public class Program
 {
 public void Run(AbstractPayCheckCalculatorFactory<PayCheckCalculator> calculatorFactory)
 {
 bool tryAgain = false;
 do
 {
 var myUserInterface = new PayCheckCalculatorView();
 Employee employee;
 try
 {
 employee = myUserInterface.GetEmployee();
 myUserInterface.ShowResult(calculatorFactory.CreatePayCheckCalculator(employee.Location).Calculate(employee.HoursWorked, employee.HourlyRate));
 }
 catch (ArgumentException ex)
 {
 if (ex.Data.Contains("errors"))
 {
 var errors = ex.Data["errors"] as ICollection<ValidationError>;
 if (errors != null)
 {
 myUserInterface.ShowErrors(errors);
 }
 }
 else
 {
 myUserInterface.ShowError(ex.Message);
 }
 }
 catch (Exception ex)
 {
 myUserInterface.ShowError(ex.Message);
 }
 finally
 {
 tryAgain = myUserInterface.IsConcluded();
 }
 }
 while (tryAgain);
 }
 static void Main(string[] args)
 {
 (new Program()).Run(new PayCheckCalculatorFactory());
 }
 }
}

So, let me explain the software architecture as I see. First, I splited the codebase in 4 different visual studio projects, as listed below:

1. Domain (Class Library):

   Contains the domain entities, as Employee, Paycheck and Deduction. It also contains a Validation class that is used to validade the state of different classes in the system and two abstract classes used as superclasses for the calculators and calculator factories.

2. PayCheckCalculators (Class Library):

   Contains the paycheck calculators. As the requirements state, each country have its own sort of deductions. However, the gross and net salary calculation is universal. Gross salary is the simple multiplication of HoursWorked and hourlyRate. The net salary is simply gross salary minus the summation of all deductions.

   Consequently, the calculation of gross and net salary is done in the abstract class PayCheckCalculator, while the deductions are calculated in the appropriate child classes: ItalyPayCheckCalculator, GermanyPayCheckCalculator and IrelandPayCheckCalculator.

   The superclass PayCheckCalculator resides in the Domain project, while the specializations reside in the PayCheckCalculators project. Here I believe I could improve the architecture creating an Interface containing all public methods of the PayCheckCalculator abstract class, and I would make this abstract class to extend the interface. So, I could put the interface in the domain project and the abstract class in the calculators project, avoiding Paycheck calculation implementation details in the domain project. Although it would be an improvement, I don't think it is a life or death case.

3. Tests project (Class Library):

   One of the requirements was to use TDD in the development process. So I began coding by the tests and in the end I covered about 97% of the codebase with unit tests.

4. PayCheckCalculatorApp (Console Application):

   This project contains the presentation part of the software. There are only two classes in the project: Program and PayCheckCalculatorView. The Program class works as a controller. It instanciates the view and ask for an Employee.

   Then the view then asks the user for the required information and creates an Employee using the provided info. Once the controller has an instance of Employee, it requests the factory to create a new PayCheckCalculator. The concrete factory is injected to the controller (I don't use DI containers). Once the proper calculator is instantiated, the controller calls the calculate method.

   Finally, the controller checks for any thrown exceptions and in case it receives one, it call the showError/ShowErrors method of the view.

Let me summing things up comparing to SOLID principles:

1. Single Responsibility

   Each class has one reason to change. For example, the Employee class can only change if the business rules change, because all it does is to validate the user inputs, as hours worked, hourly rate and location (country).

   In addition, each calculator can only change if the rules for that precise calculator change. For example, if from now on Ireland starts to charge a lower income tax or change the rules to apply this income tax, then the IrelandPayCheckCalculator would have to be update (or I new class should be designed, as I explain later). So, I believe I achieved to comply with single responsibility principle.

2. Open/Closed Principle

   Let's say I want to implement a new calculator, for example BrazilPayCheckCalculator. All I have to do is to create a subclass of PayCheckCalculator and implement the CalculateDeductions method. In addition, I would need to update the concrete factory class or create a subclass of AbstractPayCheckCalculatorFactory (or even of the concrete PayCheckCalculatorFactory) and implement the logic to return an instance of BrazilPayCheckCalculator.

   Changes can be also accomplished like in the creation of new calculators. In other words, if I need to change the requirements of a given calculator, I could simply create a new subclass of PayCheckCalculator and implement the required logic.

   Summing all these up, the code complies with open/closed principle.

3. Liskov Substitution Principle

   The Program class uses the calculator in a polymorphic way. First, it delegates to a factory class the creation of the proper calculator instance. In addition, the factory class returns the instance using the abstract superclass PayCheckCalculator. The Program class then calls the Calculate method in the returned instance.

   Consequently, we can replace this instance by any child of PayCheckCalculator and the software will work seamlessly.

   The same is not true for the relationship between the Program and View classes. However, I don't think it is a core concern.

   Consequently, to the best of my knowledge, the code complies with the Liskov Substitution principle.

4. Interface Segregation Principle

   Well, as in my code I don't have a case where a class is forced to implement methods that it doesn't use, I believe that this principle also holds in my codebase.

5. Inversion of Dependency

   This part could be improved as I said in the beginning of the post. Currently, all my calculator classes derive from an abstract class that actually implements the Gross and Net salary calculation and let the subclasses calculate the deductions.

   Consequently, the clients of a calculator don't depend on the concrete calculator, but instead depend on the abstract class. In other words, the client classes are obliviated from the specifics of each calculator. Even better, because the abstract class resides in the domain assembly, no references need to be made to the calculators assembly.

As I said before, this could be improved. Because the abstract class has some implementation details, it would be better to create an interface and make the abstract class implement (partially) it.

Because all of these, I believe my code complies with all solid principles. It is true it can be improved, but I don't believe it could be evaluated as a code written by a developer with low knowledge of OO design principles.

Answer 1

I'm sorry, I don't have it in me to read your full explanations (TLDR), but here are some questionable design choices, that caught my eye after looking through your code:

1. Calculator has no interface. I think it should. And it should be very clear. I don't want to know implementation details, I don't want to call factories and all that stuff, that doesn't even fit the screen:

    calculatorFactory.CreatePayCheckCalculator(employee.Location).Calculate(employee.HoursWorked, employee.HourlyRate)
   

   I want to give an employee and receive a pay check.

   interface IPayCheckCalculator
   {
    PayCheck Calculate(Employee employee);
   }
   //usage:
   var employee = ...;
   //no factories! Whether or not paycheck depends on location
   //and how it is calculated (per-hour rate or monthly payments or w/e) 
   //is an implementation detail!
   var payCheck = payCheckCalculator.Calculate(employee); 
   

2. Calculators implement deduction rules. Should they, really? How would you re-use a rule in different implementations? The answer is - you copy-paste, as you have demonstrated in IrelandPayCheckCalculator.CalculateIncomeDeduction , IrelandPayCheckCalculator.CalculateUniversalSocialChargeDeduction and GermanyPayCheckCalculator.CalculateIncomeDeduction. A better approach, IMHO, is to come up with a common rule-set and reuse it in your calculators. Say:

   interface IDeductionRule
   {
    void Apply(PayCheck payCheck);
   }
   class FlatDeductionRule : IDeductionRule {...}
   class PercentageDeductionRule : IDeductionRule {...}
   class SurplusPercentageDeductionRule : IDeductionRule {...}
   //etc
   

   You can easily reuse those rules by setting correct country-specific parameters (such as exact percentages or rule description) in constructor. And, even more importantly, you can now write proper unit-tests to test every rule in isolation.

3. DeductionType is enum. Why? Is it a fixed set of values, that is unlikely to change? No, it is the opposite. With every new country added, you will have to modify it, adding even more values to this already confusing list. Description property is more than enough.

4. Employee validation is part of Employee class. I think there two different approaches to validation, that serve different purpose. First is to implement validation inside the class to prevent developer from making mistakes. In this case validation rules should be simple and universal and exception type should be accurate (I want ArgumentNullException, if argument is null). Second is to implement separate validator, that would have a more complex and more specific validation rules that prevent user from making input mistakes. This validator can then be extended or replaced to further tweak its behavior depending on user environment. You go for the first approach, yet validation logic is too complex, exceptions are inaccurate and rules are not universal (for example, for some reason I am not allowed to use - or whitespace in my country's name).

5. There are some other things. PayCheck should probably be immutable. There are too many projects for such simple program (over-engineering). AbstractPayCheckCalculatorFactory<T> is useless, since it has single non-generic implementation (over-engineering). But those are minor issues.

Answer 2

@Nikita B has some good points. I'm just going to add my own thoughts on top of his, which may involve some repetition.

Deductions and DRY

I would say that you've violated the rule of DRY quite a bit. There is a lot of overlap between the kinds of deductions, but your Deduction class doesn't have any real functionality: it is just a dumb data carrier. I think a general architecture that will be easier to use/understand/support would break the deductions up by type, and then each country class would effectively just be a list of which deductions apply and how exactly they are configured. For instance, the primary deduction type would be "bracket" deduction, and you could imagine Ireland simply being the sum of 4 deductions (in pseudo-code):

public class IrelandPayCheckCalculator : PayCheckCalculator
{
 private function getDeductions()
 {
 return [
 new BracketDeduction( 'Income Tax', 0, 600, 0.25 ),
 new BracketDeduction( 'Income Tax', 601, 0, 0.40 ),
 new BracketDeduction( 'Universal Social', 0, 500, 0.07 ),
 new BracketDeduction( 'Universal Social', 501, 0, 0.08 )
 ]
 }
}

To be clear, the calling sequence for the BracketDeduction constructor is ( description, min_income, max_income, fractional_tax ), with max_income=0 implying no limit. This would then be the entirety of your IrelandPayCheckCalculator class. Each Deduction class would then be responsible for calculating the actual deduction, given the settings it was configured with when constructed plus a gross income passed to it. PayCheckCalculator would then just be responsible for storing the deductions and calling them to make the final calculations: no real calculation itself. The above example would work equally well for Germany, and then you would just have to come up with a custom deduction for Italy.

Obviously this particular implementation wouldn't improve your issues regarding inversion of control, but my point here is to highlight ways to better design your code so it repeats itself less.

General Flow of Data

I would also state the overall flow of your data is very confusing, and is going to be very hard to maintain in the long run. The short of it is that there is almost no encapsulation of data, and everyone is getting their hands dirty with everyone else's data. I'm not sure if there is really a good way to summarize this other than to point out the many ways in which it happens:

1. Your PayCheckCalculator.CalculateGrossAmount() directly sets the value of the paycheck.GrossSalary, a public property of another object. The paycheck should calculate its own gross salary, or gross salary should stay with the paycheck calculator. IMO, directly setting a public property of another class is a good way to cause maintenance issues down the road.
2. PayCheckCalculator.CalculateDeductions() is once again directly modifying the paycheck instance, this time by adding deductions onto its deductions array. This is what public getters and setters are for: they encapsulate the object data so that the class retains full control over what happens to it. A better process though would be for your calculate deductions function to simply return the deductions. Then the calling code can do whatever it wants with them.
3. You are passing along the full paycheck object to both PayCheckCalculator.CalculateDeductions() and PayCheckCalculator.CalculateIncomeDeduction(), despite the fact that all you ever use is payCheck.GrossSalary. This is creating an unnecessary coupling between these two components. Your PayCheckCalculator should really only be receiving the gross income as a number. There is no reason for it to get the entire object, only to pull the gross income out itself. If you ever decide to refactor payCheck, you can't do so without also refactoring PayCheckCalculator, but this wouldn't be the case if you only passed along the information the calculator needed rather than the full paycheck instance. Again, it's about separation of concerns and encapsulation: your classes know far too much about eachother.
4. PayCheckCalculator.CalculateNetAmount() is once again looping over the deductions that it set on the paycheck instance. You know you have a code organization problem when class A directly sets data in class B and then later on gets that data directly out of class B to use for its own purposes.

The short of it is that your classes or tightly coupled, poorly separated, and at risk of turning into a big ball of mud in the future. Some interfaces would help you: each class should have a well defined "contract" which it enforces through proper visibility rules. Never directly set public properties of another instance. I would say that you need to focus more on a clean separation of concerns, and really planning your system out before hand to make sure that each class has clearly defined responsibilities that adhere to the SRP.