Maintainability and Readability of a function which has many if else conditions-CodePudding

I have a function to calculate Salary as follows . CalculateHRA and CalculaAllowance function definitions are not shown. The CalculateSalary has a lot of if-else conditions and if any conditions gets added , this function keeps growing . Is there a better way to handle this apart from if-else?

public class SalaryDTO{
 public int Salary{get;set;}
 public int Bonus {get;set;}
 public int HRA {get;set;}
 public int Allowance {get;set;}
 public string Output{get;set;}
}

public void CalculateSalary(List<SalaryDTO> salaryDTO)
{
  foreach(var a in salaryDTO)
  {
    if(a.Salary > 0 && a.Bonus >0)
    {
      a.HRA = CalculateHRA(a.Salary,a.Bonus);
      a.Allowance = CalculateAllowance(a.Salary,a.Bonus);
      a.Output = "Profit";
    }
   if(a.Salary < 0 && a.Bonus < 0)
    {
      a.HRA = CalculateHRA(a.Salary,a.Bonus);
      a.Allowance = CalculateAllowance(a.Salary,a.Bonus);
      a.Output = "Loss";
    }
    if(a.Salary > 0 && a.Bonus == 0)
    {
      a.HRA = CalculateHRA(a.Salary,a.Bonus);
      a.Allowance = 10;
      a.Output = "Profit";
    }
   if(a.Salary == 0 && a.Bonus < 0)
    {
      a.HRA = 20;
      a.Allowance = CalculateAllowance(a.Salary,a.Bonus);
      a.Output = "Profit";
    }
    if(a.Salary <0 && a.Bonus >0)
    {
     //Somecode and conditions similar to above 
    }
  }
}

CodePudding user response：

Perhaps the first thing to consider is who is responsible for knowing how to calculate salary. Perhaps you need a Salary class whose sole responsibility is modeling the concept of "salary" in your business. That class might include a few methods to act on the data.

    public class Salary
    {
        public Salary(int salaryValue, int bonus)
        {
            SalaryValue = salaryValue;
            Bonus = bonus;
            HRA = CalculateHRA();
            Allowance = CalculateAllowance();
        }

        public int SalaryValue { get; private set; }
        public int Bonus { get; private set; }
        public int HRA { get; private set; }
        public int Allowance { get; private set; }
        public string Output => SalaryValue < 0 ? "Loss" : "Profit";

        private static int CalculateHRA()
        {
            // ...
        }

        private static int CalculateAllowance()
        {
            // ...
        }
    }

This is different than the DTO. The DTO contains data but no behavior.

To calculate the HRA and Allowance, you may need to check conditions but only those conditions that apply to this task. Also worth considering, "Should the Salary class be responsible for calculating HRA?" Maybe, maybe not. In a second round of refactoring you might try it and see if a dedicated HRA class improves reading and understanding. Same for Allowance.

With a Salary model responsible for the implementation details, you can get a list of fully calculated salaries from a list of DTOs like this:

public IEnumerable<Salary> CalculateSalary(List<SalaryDTO> salaryDTOs) =>
    salaryDTOs.Select(dto => new Salary(dto.Salary, dto.Bonus));

This is certainly not the only way to do it. It is important to have good automated unit test as a safety net to make sure the behavior is correct. Make decisions guided by simple principles like:

Encapsulating data and related methods together,
Each class or method should be responsible for only one thing.