Cfrtjryk

I develop this mini project as an exercise. It is a simplified employee salary calculator.

Requirements:

• Employees are characterized by name, employment date, employee group and base salary.


• There are three groups of employees: Employee, Manager, Salesman.


• Each employee might have a supervisor.


• Each employee except "Employee" group might have subordinates of any group.


• 
  

  Salary should be calculated the following way:

  
  
  
  
  

  • For "Employee" group - base salary + 3% for each year of work since employment date, but no more than 30% over.

  
  

  • For "Manager" group - base salary + 5% for each year of work, but no more than 40% over + 0,5% of salary of all direct(first level) subordinates.

  
  

  • For "Salesman" group - base salary + 1% for each year of work, but no more than 35% over + 0,3% of salary of ALL subordinates, direct or indirect.

  
  
  
  



• The app should allow a user to get calculated salary of any selected employee at any given moment as well as summary of ALL company employees' salaries.

My solution:

public class Employee

{

    public int Id { get; set; }

    public string Name { get; set; }

    public DateTime EmploymentDate { get; set; }

    public string EmployeeGroup { get; set; }

    public int BaseSalary { get; set; }

    public int? SupervisorId { get; set; }

    public string Lineage { get; set; }

    public int Depth { get; set; }

    public Employee Supervisor { get; set; }

    public List<Employee> Subordinates { get; set; }

}



public class EmployeesDbContext : DbContext

{

    public DbSet<Employee> Employees { get; set; }



    public EmployeesDbContext(DbContextOptions<EmployeesDbContext> options) : base(options)

    {

    }



    protected override void OnModelCreating(ModelBuilder modelBuilder)

    {

        modelBuilder.Entity<Employee>()

            .HasOne(e => e.Supervisor)

            .WithMany(e => e.Subordinates)

            .HasForeignKey(e => e.SupervisorId);

    }

}



public interface IEmployeeRepository

{

    void Add(Employee employee);

    void AddRange(IEnumerable<Employee> employees);

    void Delete(Employee employee);

    Task<List<Employee>> ListAllAsync();

    Task<Employee> FindByIdAsync(int id);

    Task<List<Employee>> GetAllSubordinatesAsync(Employee employee);

    Task<List<Employee>> GetFirstLevelSubordinatesAsync(Employee employee);

    IQueryable<Employee> GetAsQueryable();

    Task<int> GetEmployeeTreeDepth();

    Task<List<Employee>> GetAllOfLevel(int depth);

    Task SaveChangesAsync();

}



public class EmployeeRepository : IEmployeeRepository

{

    private readonly EmployeesDbContext _employeesDbContext;



    public EmployeeRepository(EmployeesDbContext dbContext)

    {

        _employeesDbContext = dbContext;

    }



    public void Add(Employee employee)

    {

        _employeesDbContext.Add(employee);

    }



    public void Delete(Employee employee)

    {

        _employeesDbContext.Remove(employee);

    }



    public void AddRange(IEnumerable<Employee> employees)

    {

        _employeesDbContext.AddRange(employees);

    }



    public async Task<List<Employee>> GetAllSubordinatesAsync(Employee employee)

    {

        var result = await _employeesDbContext.Employees

            .FromSql("SELECT * FROM Employees WHERE lineage LIKE '{0}-%'", employee.Lineage)

            .ToListAsync();



        return result;

    }



    public async Task<Employee> FindByIdAsync(int id)

    {

        return await _employeesDbContext.FindAsync<Employee>(id);

    }



    public async Task<List<Employee>> GetFirstLevelSubordinatesAsync(Employee employee)

    {

        var result = await _employeesDbContext.Employees

            .FromSql("SELECT * FROM Employees WHERE lineage LIKE '{0}-%' AND depth = {1}", employee.Lineage, employee.Depth + 1)

            .ToListAsync();



        return result;

    }



    public async Task<List<Employee>> ListAllAsync()

    {

        return await _employeesDbContext.Employees.AsNoTracking().ToListAsync();

    }



    public IQueryable<Employee> GetAsQueryable()

    {

        return _employeesDbContext.Employees.AsQueryable().AsNoTracking();

    }



    public async Task<int> GetEmployeeTreeDepth()

    {

        return await _employeesDbContext.Employees.MaxAsync(e => e.Depth);

    }



    public async Task<List<Employee>> GetAllOfLevel(int depth)

    {

        return await _employeesDbContext.Employees.Where(e => e.Depth == depth).ToListAsync();

    }



    public async Task SaveChangesAsync()

    {

        await _employeesDbContext.SaveChangesAsync();

    }





}



public interface ISalaryCalculator

{

    Task<int> CalculateSalaryAsync(Employee employee);

    Task<int> CalculateSalariesSumAsync();

}



public class SalaryCalculator : ISalaryCalculator

{

    private readonly ISalaryCalculatorFactory _salaryCalculatorFactory;

    private readonly IEmployeeRepository _employeeRepository;



    public SalaryCalculator(ISalaryCalculatorFactory salaryCalculatorFactory, IEmployeeRepository employeeRepository)

    {

        _salaryCalculatorFactory = salaryCalculatorFactory;

        _employeeRepository = employeeRepository;

    }



    public async Task<int> CalculateSalariesSumAsync()

    {

        var employees = await _employeeRepository.ListAllAsync();

        IEnumerable<Task<int>> calcSalaryTasksQuery = employees.Select(e => CalculateSalaryAsync(e));

        Task<int> calcSalaryTasks = calcSalaryTasksQuery.ToArray();

        int salaries = await Task.WhenAll(calcSalaryTasks);

        var salarySum = salaries.Sum();



        return salarySum;

    }



    public async Task<int> CalculateSalaryAsync(Employee employee)

    {

        var calculator = _salaryCalculatorFactory.CreateCalculator(employee.EmployeeGroup);

        var salary = await calculator.CalculateSalaryAsync(employee);

        return salary;

    }

}



public interface ISalaryCalculatorFactory

{

    BaseSalaryCalculator CreateCalculator(string employeeGroup);

}



public class SalaryCalculatorFactory : ISalaryCalculatorFactory

{

    private Dictionary<string, Func<BaseSalaryCalculator>> salaryCalculators;





    public SalaryCalculatorFactory(IEmployeeRepository employeeRepository)

    {

        salaryCalculators = Assembly.GetExecutingAssembly().GetTypes()

            .Where(t => typeof(BaseSalaryCalculator).IsAssignableFrom(t) && t.IsAbstract == false)

            .Select(t => new Func<BaseSalaryCalculator>(() => Activator.CreateInstance(t,employeeRepository, this) as BaseSalaryCalculator))

            .ToDictionary(f => f().EmployeeGroup);

    }



    public BaseSalaryCalculator CreateCalculator(string employeeGroup)

    {

        return salaryCalculators[employeeGroup]();

    }

}



public abstract class BaseSalaryCalculator

{

    public abstract string EmployeeGroup { get; }

    protected readonly IEmployeeRepository _employeeRepository;

    protected readonly ISalaryCalculatorFactory _salaryCalculatorFactory;



    protected BaseSalaryCalculator(IEmployeeRepository employeeRepository, ISalaryCalculatorFactory salaryCalculatorFactory)

    {

        _employeeRepository = employeeRepository;

        _salaryCalculatorFactory = salaryCalculatorFactory;

    }



    public abstract Task<int> CalculateSalaryAsync(Employee employee);



    protected int CalcEmployeeExperience(DateTime employmentDate)

    {

        return (int)Math.Floor(DateTime.Now.Subtract(employmentDate).TotalDays / 365);

    }



    protected int CalcExperiencePremium(int baseSalary, int premiumPercentForEachYearExp, int maxExperiencePremiumPercent, int experience)

    {

        var experiencePremium = baseSalary / 100 * premiumPercentForEachYearExp;

        var maxExpPremium = baseSalary / 100 * maxExperiencePremiumPercent;

        var premium = experiencePremium * experience;

        if (premium > maxExpPremium) premium = maxExpPremium;

        return premium;

    }



    protected async Task<int> CalcSupervisorPremium(IEnumerable<Employee> subordinates, float supervisorPremiumPercent)

    {

        int salarySum = 0;

        foreach (var employee in subordinates)

        {

            var calculator = _salaryCalculatorFactory.CreateCalculator(employee.EmployeeGroup);

            var salary = await calculator.CalculateSalaryAsync(employee);

            salarySum += salary;

        }

        var premium = (int)Math.Ceiling(salarySum / 100 * supervisorPremiumPercent);

        return premium;

    }

}



public class EmployeeSalaryCalculator : BaseSalaryCalculator

{

    public override string EmployeeGroup => "Employee";

    private const int _premiumPercentForEachYearExp = 3;

    private const int _maxExperiencePremiumPercent = 30;



    public EmployeeSalaryCalculator(IEmployeeRepository employeeRepository, ISalaryCalculatorFactory salaryCalculatorFactory) 

        : base(employeeRepository, salaryCalculatorFactory)

    {

    }



    public override async Task<int> CalculateSalaryAsync(Employee employee)

    {

        var salary = await Task.Run(() =>

        {

            var experience = CalcEmployeeExperience(employee.EmploymentDate);

            var experiencePremium = CalcExperiencePremium(employee.BaseSalary, _premiumPercentForEachYearExp, _maxExperiencePremiumPercent, experience);

            var totalSalary = employee.BaseSalary + experiencePremium;

            return totalSalary;

        });

        return salary;

    }

}



public class ManagerSalaryCalculator : BaseSalaryCalculator

{

    public override string EmployeeGroup => "Manager";

    private const int _premiumPercentForEachYearExp = 5;

    private const int _maxExperiencePremiumPercent = 40;

    private const float _supervisorPremiumPercent = 0.5f;



    public ManagerSalaryCalculator(IEmployeeRepository employeeRepository, ISalaryCalculatorFactory salaryCalculatorFactory) 

        : base(employeeRepository, salaryCalculatorFactory)

    {

    }



    public override async Task<int> CalculateSalaryAsync(Employee employee)

    {

        var experience = CalcEmployeeExperience(employee.EmploymentDate);

        var experiencePremium = CalcExperiencePremium(employee.BaseSalary, _premiumPercentForEachYearExp, _maxExperiencePremiumPercent, experience);

        var subordinates = await _employeeRepository.GetFirstLevelSubordinatesAsync(employee);

        var supervisorPremium = await CalcSupervisorPremium(subordinates, _supervisorPremiumPercent);

        var totalSalary = employee.BaseSalary + experiencePremium + supervisorPremium;

        return totalSalary;

    }

}



public class SalesmanSalaryCalculator : BaseSalaryCalculator

{

    public override string EmployeeGroup => "Salesman";

    private const int _premiumPercentForEachYearExp = 1;

    private const int _maxExperiencePremiumPercent = 35;

    private const float _supervisorPremiumPercent = 0.3f;



    public SalesmanSalaryCalculator(IEmployeeRepository employeeRepository, ISalaryCalculatorFactory salaryCalculatorFactory) 

        : base(employeeRepository, salaryCalculatorFactory)

    {

    }



    public override async Task<int> CalculateSalaryAsync(Employee employee)

    {

        var experience = CalcEmployeeExperience(employee.EmploymentDate);

        var experiencePremium = CalcExperiencePremium(employee.BaseSalary, _premiumPercentForEachYearExp, _maxExperiencePremiumPercent, experience);

        var subordinates = await _employeeRepository.GetAllSubordinatesAsync(employee);

        var supervisorPremium = await CalcSupervisorPremium(subordinates, _supervisorPremiumPercent);

        var totalSalary = employee.BaseSalary + experiencePremium + supervisorPremium;

        return totalSalary;

    }

}

As you can see I ended up with a kind of circular dependency between SalaryCalculatorFactory and specific calculators it creates. And I don't know how to solve this elegantly and avoid duplicate code in CalcSupervisorPremium method without creating the circular dependency and tight coupling (or it is okay and there's a good way to call SalaryCalculator's method, I don't know).
It seems this circularity is a natural part of the algorithm since to calculate some managers' and salesmans' salaries we need to calculate salaries of their subordinates.

So, how to improve the design?

You should use decimal or at least double as data type for financial figures. You usually operate with (at least) two decimals in financial calculations.

IEmployeeRepository should inherit from IDisposable to force implementers to implement IDisposable to dispose of the EmployeesDbContext instance.
And that means that ISalaryCalculator should do the same in order to dispose of the EmployeeRepository instance.

I'm not sure I understand the "Lineage" concept. To me it looks like you
maintain two parent-child relationships on the same objects: Subordinaries and Lineage (and what role has the SupervisorId
in that equation?) Why don't you rely on the hierarchical
relationship through Subordinates (maintained through the navgation property(ies))?

There is a "conceptual meltdown" in having CalcSupervisorPremium(...) as a member of the BaseSalaryCalculator
because it is a specialization of the common salary calculation - related only to certain employee types.
I would create an abstract class SupervisorSalaryCalculator : BaseSalaryCalculator as base class for ManagerSalaryCalculator and SalesmanSalaryCalculator
and because the two sub classes are almost identical except for the values of their members you can let the new base class do the calculations:

  public abstract class SupervisorSalaryCalculator : BaseSalaryCalculator

  {

    public override string EmployeeGroup { get; }

    private int PremiumPercentForEachYearExp { get; }

    private int MaxExperiencePremiumPercent { get; }

    private float SupervisorPremiumPercent { get; }



    public SupervisorSalaryCalculator(

      string employeeGroup, 

      int experienceRate, 

      int experienceRateMax,

      float supervisorRate,

      IEmployeeRepository employeeRepository, 

      ISalaryCalculatorFactory salaryCalculatorFactory)

        : base(employeeRepository, salaryCalculatorFactory)

    {

      EmployeeGroup = employeeGroup;

      PremiumPercentForEachYearExp = experienceRate;

      MaxExperiencePremiumPercent = experienceRateMax;

      SupervisorPremiumPercent = supervisorRate;

    }



    protected async Task<int> CalcSupervisorPremium(IEnumerable<Employee> subordinates, float supervisorPremiumPercent)

    {

      int salarySum = 0;

      foreach (var employee in subordinates)

      {

        var calculator = _salaryCalculatorFactory.CreateCalculator(employee.EmployeeGroup);

        var salary = await calculator.CalculateSalaryAsync(employee);

        salarySum += salary;

      }

      var premium = (int)Math.Ceiling(salarySum / 100 * supervisorPremiumPercent);

      return premium;

    }



    async public override Task<int> CalculateSalaryAsync(Employee employee)

    {

      var experience = CalcEmployeeExperience(employee.EmploymentDate);

      var experiencePremium = CalcExperiencePremium(employee.BaseSalary, PremiumPercentForEachYearExp, MaxExperiencePremiumPercent, experience);

      var subordinates = await GetSubordinatesAsync(employee);

      var supervisorPremium = await CalcSupervisorPremium(subordinates, SupervisorPremiumPercent);

      var totalSalary = employee.BaseSalary + experiencePremium + supervisorPremium;

      return totalSalary;

    }



    abstract protected Task<List<Employee>> GetSubordinatesAsync(Employee employee);

  }



  public class ManagerSalaryCalculator : SupervisorSalaryCalculator

  {

    public ManagerSalaryCalculator(IEmployeeRepository employeeRepository, ISalaryCalculatorFactory salaryCalculatorFactory)

        : base("Manager", 5, 40, 0.5f, employeeRepository, salaryCalculatorFactory)

    {

    }



    async protected override Task<List<Employee>> GetSubordinatesAsync(Employee employee)

    {

      return await _employeeRepository.GetFirstLevelSubordinatesAsync(employee);

    }

  }



  public class SalesmanSalaryCalculator : SupervisorSalaryCalculator

  {

    public SalesmanSalaryCalculator(IEmployeeRepository employeeRepository, ISalaryCalculatorFactory salaryCalculatorFactory)

        : base("Salesman", 1, 35, 0.3f, employeeRepository, salaryCalculatorFactory)

    {

    }



    async protected override Task<List<Employee>> GetSubordinatesAsync(Employee employee)

    {

      return await _employeeRepository.GetAllSubordinatesAsync(employee);

    }

  }

The calculation of years of employment is not always correct.
Try for instance employmentDate = new DateTime(2000, 12, 3) and now = new DateTime(2018, 11, 29)
It will give 18 years, where it should give 17 (whole) years, while employmentDate = new DateTime(2000, 12, 7)
gives the correct 17 for the same now value. 365 is not a reliable number of days per year.

Instead you can do something like this:

   years = now.Year - employmentDate.Year;

   if (now.Date < employmentDate.AddYears(years).Date)

     years--;

   return years;

Not very fancy but more reliable.

public SalaryCalculatorFactory(IEmployeeRepository employeeRepository)

{

    salaryCalculators = Assembly.GetExecutingAssembly().GetTypes()

        .Where(t => typeof(BaseSalaryCalculator).IsAssignableFrom(t) && t.IsAbstract == false)

        .Select(t => new Func<BaseSalaryCalculator>(() => Activator.CreateInstance(t,employeeRepository, this) as BaseSalaryCalculator))

        .ToDictionary(f => f().EmployeeGroup);

}

To investigate all types in an assembly to find the few you can use may be an expensive task every time the salary factory is created. If working
in a large assembly with lots of types, that may be a bottleneck. You'll have to measure on that. You could consider to make the calculator dictionary static, so it is only loaded once, or
have the calculators in a dedicated assembly, and/or read the types from a configuration file.

Your naming is very descriptive, but some names maybe a little too much: premiumPercentForEachYearExp;
maybe experienceRate or the like would be descriptive enough? IMO both abbreviated and too long names influence the readability negatively.

More about naming:

You have SalaryCalculator which is the main "interface" for clients to use and then you have BaseSalaryCalculator etc. which takes care of the
actual calculations. Maybe a little confusing with the similar names, when they are not directly related? - if confused me at least.

According to the overall design and structure, you show good understanding of dependency injection, dependency inversion, repository and factory patterns
and having these concepts at hand is very useful when it comes to larger projects. But here the amount of code lines seems rather overwhelming, which is increased by your rather verbose naming style.

But then again the overall impression is a code that is thought through and well-structured.

Think separation of concerns

I ended up with a kind of circular dependency between SalaryCalculatorFactory and specific calculators

The SalaryCalculator design conflates its proper functions with the using/client code. That is why you have that dependency problem. Think of objects as doing a discrete thing and then separately think about the code using it.

Factory

Keeping a factory reference in the class it creates doesn't make sense. A factory builds stuff, that's all. It should not keep anything on behalf of what it creates. Just like after a car rolls off the assembly line, the factory has nothing to do with it. There are will be other entities for that.

Tell the factory what to build and have that returned; done. If a collection class holding all the instantiated calculators is needed then make one, don't use the factory for that - it's not a warehouse.

enum is your friend

Use enum to define employee groups. It avoids all the problems using strings. enum is type safe, and best of all it shows up in intellisense. Typos are caught at compile time. enum has a very documenting quality. Here, you're declaring all employee groups that exist, named "EmployeeGroup", and very significantly in one place.

Zombie interfaces

The code smells:

• The fact that there are specific custom calculators tells me that all those ("capital I") interfaces are not used effectively.


• a general/abstract "calculate" method is not defined anywhere.


• object reference variables are not of the interface types.

My Spidey Sense ™ tells me you are thinking you must use interfaces and this imperative is over-powering coherent, sensible design. The principle code to interfaces not implementation does not mean only "capital I" interface. "interface" can mean an interface, abstract class, delegates and even, I do dare say, a concrete class.

interface is not needed in the calculator design. Get rid of them.

Do not imbed implementation details in variable names. Do.not.do.this. Do not use type either - no "intSalary", "objEmployee", etc - you don't do that, I'm just saying. Good class names and use-variable names give plenty of context. client code should not know and does not care about these things.

CalculateSalariesSumAsync();   // no

CalculateSalaries();             // much better

Calculator Design

Insight: Calculators are all the same, they all CalcSalary(). Only one class needed. We just need a way to insert the one thing that does change - the calculating algorithm code.

Insight: Don't try to design everything all at once. The basic calculator here, I'll worry about the factory next. I'll think about how to get calculators and employee lists [do not imply any specific implementation] together later.

Insight: You will modify classes as design progresses. Don't worry about it.

So I'm picturing this:

  SalaryFactory salaryGenerator = new SalaryFactory();

  SalaryCalculator employeeCalculator = salaryGenerator.Create( EmployeeGroup.Employee );

  SalaryCalculator managerCalculator = salaryGenerator.Create( EmployeeGroup.Manager );

  SalaryCalculator salesmanCalculator = salaryGenerator.Create( EmployeeGroup.Salesman ); 

And a couple possibilities for SalaryCalculator class:

Oh, Look Mo! A concrete class as interface.

public class SalaryCalculator {

    public SalaryCalculator(Func<decimal> algorithm){

       SalaryAlgorithm = algorithm;

    }

    public Func<decimal> CalcSalary ( ) { return SalaryAlgorithm(); }



    protected Func<decimal> SalaryAlgorithm;   // the factory supplies this

}

I like abstract classes a lot:

public abstract class SalaryCalculator {



    abstract public Func<decimal> CalcSalary ( ) { ... }  // the factory supplies this

}

interface, abstract class, concrete class

First, it all depends on overall design. Nothing is absolute.

Generally an interface is for giving unrelated classes common behavior. With inheritance an abstract class is my first choice. A concrete class is a valid choice because we can still use composition, that's what constructors are for.

abstract class allows for both base implementation and "placeholder" declarations a-la interface. Mixing these two features in the code flow/logic is called a Template Pattern (google it).

Using interface and then hoping every class implements base functionality the same is delusional.

group salaries

Group functionality is distinct from individual employee functionality and should be a separate class, even if group salary is the only group function (for now).

The goal is a general Employees class that can participate in a composition to calculate manager salaries.

single responsibility

• A factory should only create objects.


• A employee class should be only about and directly about an individual employee.


• A calculator should only calculate salary and not care who, what, where, why it's calculate function was called.


• Group functionality uses a composition of these "individual" objects and are their own classes.

Uh-oh! Bob is suggesting more classes. Yes. Yes I am. This is the nature good OO design. You end up with classes doing higher level/complex things which are compositions of more basic classes, every class lazer focused on doing it's own thing.

Employees

I see a general Employees class that contains a List<Employee> with functionality like this:

public class Employees {

   // List may or may not be the ideal choice for the collection. It depends.   

   protected List<Employee> Employees {get; set;}



   public decimal GroupSalary() {

       var decimal groupSalary = 0;



       forEach(var employee in this.Employees) {

           groupSalary += employee.Salary();

       }

       return groupSalary;

   }



  // all other group functionality as needed.

}

I did not create a GroupSalary property, private or otherwise. A calculated value should always be re-calculated when used/called. I guarantee future problems if you don't.

Pending further design analysis I don't see a need for a GroupSalaryCalculator.

static

Given overall single responsibility application consider making the factory and generated calculators static.

static objects should not hold state for external objects.