2
\$\begingroup\$

My thoughts are to create a method that contains all of the variables declared. Is it better to say initialized?

I would like it to be more efficient but still as clear as possible for future maintenance of the code. Any suggestions for the main bulk of information, such as methods?

if (incomeDec <= 300)
{
    taxRateDec1 = 0.15m;
    taxDec = incomeDec * taxRateDec1;
}
else if (incomeDec <= 450)
{
    tierAmtInt = 300;
    taxRateDec1 = 0.15m;
    taxRateDec2 = 0.2m;

    tempValueDec = tierAmtInt * taxRateDec1;
    taxDec = (incomeDec - tierAmtInt) * taxRateDec2;
    taxDec = taxDec + tempValueDec;
}
else
{
    tierAmtInt = 300;
    tierAmtInt2 = 150;
    taxRateDec1 = 0.15m;
    taxRateDec2 = 0.2m;
    taxRateDec3 = 0.25m;

    tempValueDec = tierAmtInt * taxRateDec1;
    tempValueDec = tempValueDec + tierAmtInt2 * taxRateDec2;
    taxDec = (incomeDec - tempValueDec) * taxRateDec3;
    taxDec = taxDec + tierAmtInt;
}

incomeDec = incomeDec - taxDec;
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

If you want more maintainable code where you can easily add more calculations as needed, you may want to refactor it to something like this, where you have a dictionary of calc-functions that you get for each case: (it's not perfect yet, but it should show you the idea)

public const decimal tierAmt1 = 300m;
public const decimal tierAmt2 = 150m;

public const decimal taxRate1 = 0.15m;
public const decimal taxRate2 = 0.2m;
public const decimal taxRate3 = 0.25m;

private IDictionary<decimal, Func<decimal, decimal>> _calcFuncs = new Dictionary<decimal, System.Func<decimal, decimal>>;

private decimal CalcIncome(decimal income)
{

    // you can of course initialize this only once in a constructor if you like
    _calcFuncs[300] = new Func<decimal, decimal>(CalcIncome1);
    _calcFuncs[450] = new Func<decimal, decimal>(CalcIncome2);
    _calcFuncs[decimal.MaxValue] = new Func<decimal, decimal>(CalcIncome3);

    // replaces all if's
    var calcFunc = _calcFuncs.First(k => income <= k.Key).Value;
    return calcFunc(income);
}

private Decimal CalcIncome1(Decimal income)
{
    var tax = 0m;
    tax = income * taxRate1;
    return income = income - tax;
}

private Decimal CalcIncome2(Decimal income)
{
    var tax = 0m;
    var tempValue = 0m;

    tempValue = tierAmt1 * taxRate1;
    tax = (income - tierAmt1) * taxRate2;
    tax = tax + tempValue;

    return income = income - tax;
}

private Decimal CalcIncome3(Decimal income)
{
    var tax = 0m;
    var tempValue = 0m;

    tempValue = tierAmt1 * taxRate1;
    tempValue = tempValue + tierAmt2 * taxRate2;
    tax = (income - tempValue) * taxRate3;
    tax = tax + tierAmt1;

    return income = income - tax;
}

CalcIncome(150);
CalcIncome(320);
CalcIncome(540);

  • You don't need the Dec and Int suffixes; make your variables harder to read and understand.
\$\endgroup\$
2
  • \$\begingroup\$ Would a dictionary be preferable to a hashtable? \$\endgroup\$
    – saechaoc
    Commented Oct 30, 2015 at 5:56
  • \$\begingroup\$ @user3167172 actually yes because a dictionary is generic and you don't have to cast keys and values to decimals and funcs, if you used a hashtable it can store only object so casting would be necessary. \$\endgroup\$
    – t3chb0t
    Commented Oct 30, 2015 at 6:01
1
\$\begingroup\$

I don't like c-style of declaring all the variables at the start of a function (which is a necessity in c, not in c#). It's better to define them closer to where they are used.

I suggest not to define your types explicitly, use var instead such as:

var tierAmtInt = 300;

You'll see the benefits when you want to modify or refactor stuff. This one looks like a good read on that: https://softwareengineering.stackexchange.com/questions/42863/explicitly-defining-variable-data-types-vs-using-the-keyword-var

For taxRateDec1/2/3, you can perhaps consider using array or sth similar.

If you really want to be perfectionist, you can consider passing all these variables as parameters or read from somewhere else. Just consider one day these rates changing, would you like to modify the code and recompile?

Last but not least, these variables are actually constants, consider putting 'const' keyword whenever necessary..

\$\endgroup\$
3
  • \$\begingroup\$ oh, and I would merge some of these calculations to a single liner, taxDec for instance.. \$\endgroup\$
    – mentat
    Commented Oct 30, 2015 at 5:19
  • \$\begingroup\$ would it be better to declare the variables in the main method and then to pass them as parameters? There are a lot of parameters and if this cchanges in the future wouldn't we have to fix that? The main goal I have is readability and maintainability but I would like it still to be efficient. I do think creating an array was helpful though! \$\endgroup\$
    – saechaoc
    Commented Oct 30, 2015 at 5:44
  • \$\begingroup\$ I would pass them from outside (main). Also you can use a struct to bind all relevant variables together, less clutter.. msdn.microsoft.com/en-us/library/ah19swz4.aspx \$\endgroup\$
    – mentat
    Commented Oct 30, 2015 at 6:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.