Progressive tax program in C#

Question

Please review this code. It should calculate the tax precentage a person will pay in a progressive tax system. The goal is a perfect program.

Comments please.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace ConsoleApplication1
{
    class TaxLevel
    {
        private decimal _Money=-1;
        /// <summary>
        /// the money count that the tax applies to
        /// </summary>
        public decimal Money
        {
            get { return _Money; }
            set { if (value > 0) { _Money = value; } else { throw new ArgumentOutOfRangeException("Money must be grater than 0"); } }
        }


        private decimal _TaxDeduction=-1;
        /// <summary>
        /// the tax level where 1.0 is 100% tax
        /// </summary>
        public decimal TaxDeduction
        {
            get { return _TaxDeduction; }
            set { if (value > 0 && value < 1.0m) { _TaxDeduction = value; } else { throw new ArgumentOutOfRangeException("TaxDeduction must be between 0.0 and 1.0"); } }
        }

     }

     class TaxCalculator
    {
        List<TaxLevel> _TaxLevels;

        public bool DeterminateTaxLevels(){

            _TaxLevels = new List<TaxLevel>();
            _TaxLevels.Add(new TaxLevel {Money = 4770m, TaxDeduction =  0.10m });
            _TaxLevels.Add(new TaxLevel { Money = 3700, TaxDeduction = 0.14m });
            _TaxLevels.Add(new TaxLevel { Money = 4249m, TaxDeduction = 0.20m });
            _TaxLevels.Add(new TaxLevel { Money = 5529m, TaxDeduction = 0.28m });
            _TaxLevels.Add(new TaxLevel { Money = 21089m, TaxDeduction = 0.31m });
            _TaxLevels.Add(new TaxLevel { Money = decimal.MaxValue, TaxDeduction = 0.42m });
        return true;
        }
         /// <summary>
         /// Gets Salley In Bruto Calculates Netto Sallery
         /// </summary>
         /// <param name="SalleryInNis">Employee Bruto Sallery</param>
         /// <returns>Neto Sallery</returns>
        public  decimal CalculateNetto(decimal SalleryInNis)
        {
            if (SalleryInNis < 0) throw new ArgumentOutOfRangeException("SalleryInNis must be greater than 0");
            if (_TaxLevels == null) throw new ArgumentNullException("DeterminateTaxLevels First");
            decimal TmpSallery =SalleryInNis;
            decimal Netto = 0.0m;   
            foreach (var taxlevel in _TaxLevels)
            {
                if (TmpSallery <= 0) break;
                if (TmpSallery > taxlevel.Money)
                {
                    Netto += taxlevel.Money * (1-taxlevel.TaxDeduction);         
                }
                else {
                    Netto += TmpSallery * (1-taxlevel.TaxDeduction);
                }
                TmpSallery -= taxlevel.Money;
            }
            return Netto;
        }
         /// <summary>
         /// Calculates Average Tax for employee
         /// </summary>
        /// <param name="SalleryInNis">Employee Bruto Sallery</param>
        /// <returns>AverageTax</returns>
        public  decimal CalculateAverageTax(decimal SalleryInNis)
        { 
            if (SalleryInNis < 0) throw new ArgumentOutOfRangeException("SalleryInNis must be greater than 0");
            return 1- (CalculateNetto(SalleryInNis) / SalleryInNis);
        }
    }
    class Program
    {
        static void Main(string[] args)
        {
            decimal Sal= 11712000;
            TaxCalculator TaxCalculator = new TaxCalculator();
            TaxCalculator.DeterminateTaxLevels();
            Console.WriteLine("Brutto " + Sal);
            Console.WriteLine("Netto " + TaxCalculator.CalculateNetto(Sal));
            Console.WriteLine("Average Tax " + TaxCalculator.CalculateAverageTax(Sal));
            Console.Read();
        }
    }
}

Answer 1

Define "perfect program". If it works every time and does not have any bugs then I say it is perfect.

Having said that, if you are looking for ways that you may improve the code...

• Your code does not following coding standards... Not that everyone follows the same set of standards but your code seems to miss quite a few. Coding standards are important because when people review your code or maintain it in the future it is easier and less frustrating for them to read it from the start.

• DetermineTaxLevels contains hard-coded values. Will the values always be the same or can they change in the future. If they can change then typically these values would be pulled from an outside source, such as an XML file or a SQL database. If they will never change then they should be defined as constant values at the top of your class or possibly in another class if they may be shared by other classes.

• Several other methods contain hard-coded values (magic numbers). You should consider putting these into constants as well.

• I do like that you have broken your class into separate methods instead of one large method. Seems like you have a good separation of concerns.

• I also like that you are guarding against invalid values.

Hope this helps give you some things to consider.

UPDATE: To respond to your comment...

Here are the items I would change. You are welcome to take or leave these suggestions as you wish...

• Although I have seen several different guidelines about naming class level variables, I mostly see _camelCase (sometimes m_camelCase). So the private decimal _Money would generally be private decimal _money or private decimal m_money. Interesting enough Microsoft recommends not prefixing them with anything so it would just be money. They then go further to say the keyword .this should always be used when you refer to them within methods. I really don't like this standard because then you have your code cluttered with this. everywhere. I think most people agree with me because I mostly see class variables named in this manner _camelCase.

• Local variables should be camelCase. So in the CalculateNetto method the variable TmpSallery should be tmpSallery

• Find a better place to guard your class variables instead of inside methods. Guarding class level variables inside a local method would mean you would have to do it in every method they are used.

• Main method should be at top.

• Do not put too much information on one line of code, such as your setters.

• Put your guard clauses at the top of a code block.

• If you want your code to be testable then you will want to implement interfaces and injects the interface into your methods (I am not showning the Interfaces here).

• I personally like to store a method call return value into a variable to help with debugging.
  so

return 1 - (CalculateNetto(salleryInNis) / salleryInNis);

would be

var netto = CalculateNetto(salleryInNis);
return 1 - (netto / salleryInNis);

Taking all these things together...if I was to writing this here is the way it would look...

using System;

namespace ConsoleApplication1
{
    class Program
    {
        private const decimal SOME_VALUE = 11712000;
        static void Main(string[] args)
        {

            decimal sal = SOME_VALUE;
            ITaxCalculator taxCalculator = new TaxCalculator();
            taxCalculator.DeterminateTaxLevels();
            Console.WriteLine("Brutto " + sal);
            Console.WriteLine("Netto " + taxCalculator.CalculateNetto(sal));
            Console.WriteLine("Average Tax " + taxCalculator.CalculateAverageTax(sal));
            Console.Read();
        }
    }

    class TaxLevel : ITaxLevel
    {
        private const decimal MIN_TAX_DEDUCTION = 0;
        private const decimal MAX_TAX_DEDUCTION = 1;

        private decimal _money = -1;

        /// <summary>
        /// the money count that the tax applies to
        /// </summary>
        public decimal Money
        {
            get { return _money; }
            set
            {
                if (value <= 0)
                    throw new ArgumentOutOfRangeException("Money must be grater than 0");

                _money = value;
            }
        }

        private decimal _taxDeduction = -1;
        /// <summary>
        /// the tax level where 1.0 is 100% tax
        /// </summary>
        public decimal TaxDeduction
        {
            get { return _taxDeduction; }
            set
            {
                if (value <= MIN_TAX_DEDUCTION || value > MAX_TAX_DEDUCTION)
                    throw new ArgumentOutOfRangeException("TaxDeduction must be between 0.0 and 1.0");

                _taxDeduction = value;
            }
        }

    }

    class TaxCalculator : ITaxCalculator
    {
        IList<ITaxLevel> _taxLevels = new List<ITaxLevel>();;

        public bool DeterminateTaxLevels()
        {
            // Too lazy to remove these magic numbers but they should be turned into constants
            _taxLevels.Add(new TaxLevel { Money = 4770m, TaxDeduction = 0.10m });
            _taxLevels.Add(new TaxLevel { Money = 3700, TaxDeduction = 0.14m });
            _taxLevels.Add(new TaxLevel { Money = 4249m, TaxDeduction = 0.20m });
            _taxLevels.Add(new TaxLevel { Money = 5529m, TaxDeduction = 0.28m });
            _taxLevels.Add(new TaxLevel { Money = 21089m, TaxDeduction = 0.31m });
            _taxLevels.Add(new TaxLevel { Money = decimal.MaxValue, TaxDeduction = 0.42m });
            return true;
        }

        /// <summary>
        /// Gets Salley In Bruto Calculates Netto Sallery
        /// </summary>
        /// <param name="SalleryInNis">Employee Bruto Sallery</param>
        /// <returns>Neto Sallery</returns>
        public decimal CalculateNetto(decimal salleryInNis)
        {
            if (salleryInNis < 0) 
                throw new ArgumentOutOfRangeException("SalleryInNis must be greater than 0");

            if (_taxLevels == null) 
                throw new ArgumentNullException("DeterminateTaxLevels First");

            decimal tmpSallery = salleryInNis;
            decimal netto = MIN_TAX_DEDUCTION;
            foreach (var taxlevel in _taxLevels)
            {
                if (tmpSallery <= 0) break;
                if (tmpSallery > taxlevel.Money)
                {
                    Netto += taxlevel.Money * (1 - taxlevel.TaxDeduction);
                }
                else
                {
                    Netto += TmpSallery * (1 - taxlevel.TaxDeduction);
                }
                tmpSallery -= taxlevel.Money;
            }
            return netto;
        }

        /// <summary>
        /// Calculates Average Tax for employee
        /// </summary>
        /// <param name="SalleryInNis">Employee Bruto Sallery</param>
        /// <returns>AverageTax</returns>
        public decimal CalculateAverageTax(decimal salleryInNis)
        {
            if (salleryInNis < 0) 
                throw new ArgumentOutOfRangeException("salleryInNis must be greater than 0");

            return 1 - (CalculateNetto(salleryInNis) / salleryInNis);
        }
    }
}

Answer 2

• If something is mandatory then use private setters and constructor arguments, and validate in the constructor (TaxLevel) [1];
• Use collection initializer notation [2];
• DeterminateTaxLevels should be private and called from the constructor (I would actually make it a simple attribution in the constructor);
• DeterminateTaxLevels should have a better name like LoadTaxLevels, because it does not "determine" anything;
• DeterminateTaxLevels has no reason to return bool - why does it?;
• Salary is misspelt;
• Either call the salary "bruto" or "in Nis" - you should be consistent, and I advise "bruto" because it means more than some random TLA like NIS (whatever it is);
• As @Gene S said, try to follow coding standards: use brackets in ifs, use line-breaks after ifs, use camelCase on parameter names and not PascalCase, etc...

---

footnotes

[1]:

private class TaxLevel
{
    /// <summary>
    /// The money count that the tax applies to.
    /// </summary>
    public decimal Money { get; private set; }
    /// <summary>
    /// The tax level where 1.0 is 100% tax.
    /// </summary>
    public decimal TaxDeduction { get; private set; }

    public TaxLevel(decimal money, decimal taxDeduction) {
        if (money < 0 ) {
            throw new ArgumentOutOfRangeException("Money must be grater than 0");
        }
        if (taxDeduction < 0 || taxDeduction > 1.0m) {
            throw new ArgumentOutOfRangeException("TaxDeduction must be between 0.0 and 1.0");
        }
        Money = money;
        TaxDeduction = taxDeduction;
    }
}

[2]:

_TaxLevels = new List<TaxLevel>() {
    new TaxLevel {Money = 4770m, TaxDeduction =  0.10m },
    new TaxLevel { Money = 3700, TaxDeduction = 0.14m },
    // ...
    new TaxLevel { Money = decimal.MaxValue, TaxDeduction = 0.42m },
};