HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Progressive tax program

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
taxprogramprogressive

Problem

Please review this code, which should calculate the tax percentage a person will pay in a progressive tax system. The goal is to have a perfect program.

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

namespace ConsoleApplication1
{
class TaxLevel
{
private decimal _Money=-1;
///
/// the money count that the tax applies to
///
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;
///
/// the tax level where 1.0 is 100% tax
///
public decimal TaxDeduction
{
get { return _TaxDeduction; }
set { if (value > 0 && value _TaxLevels;

public bool DeterminateTaxLevels(){

_TaxLevels = new List();
_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;
}
///
/// Gets Salley In Bruto Calculates Netto Sallery
///
/// Employee Bruto Sallery
/// Neto Sallery
public decimal CalculateNetto(decimal SalleryInNis)
{
if (SalleryInNis taxlevel.Money)
{
Netto += taxlevel.Money * (1-taxlevel.TaxDeduction);
}
else {
Netto += TmpSallery * (1-taxlevel.

Solution


  • 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
{
    /// 
    /// The money count that the tax applies to.
    /// 
    public decimal Money { get; private set; }
    /// 
    /// The tax level where 1.0 is 100% tax.
    /// 
    public decimal TaxDeduction { get; private set; }
    
    public TaxLevel(decimal money, decimal taxDeduction) {
        if (money  1.0m) {
            throw new ArgumentOutOfRangeException("TaxDeduction must be between 0.0 and 1.0");
        }
        Money = money;
        TaxDeduction = taxDeduction;
    }
}


[2]:

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

Code Snippets

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;
    }
}
_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 },
};

Context

StackExchange Code Review Q#17918, answer score: 5

Revisions (0)

No revisions yet.