patterncsharpMinor
Progressive tax program
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.
```
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];
DeterminateTaxLevelsshould be private and called from the constructor (I would actually make it a simple attribution in the constructor);
DeterminateTaxLevelsshould have a better name likeLoadTaxLevels, because it does not "determine" anything;
DeterminateTaxLevelshas 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.