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

"Do you owe the government?" classes

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

Problem

Should my class pass parameters internally or reference class level scoped variables?

I'm not sure on the best approach or style for procedure calls that have parameters. Should I go with the class level scoped variables?

public class YouOweTheGovernment
{
    public float AmountToPay { get; private set; }
    public float ArbitraryTaxRate { get; private set; }
    public float Salary { get; private set; }

    public YouOweTheGovernment(float taxRate, float salary)
    {
        this.ArbitraryTaxRate = taxRate;
        this.Salary = salary;

        CalculateAmount();
    }

    private void CalculateAmount()
    {
        this.AmountToPay = (this.Salary * (this.ArbitraryTaxRate / 100));
    }
}


Or explicitly pass parameters into a procedure?

public class YouOweTheGovernment
{
    public float AmountToPay { get; private set; }
    public float ArbitraryTaxRate { get; private set; }
    public float Salary { get; private set; }

    public YouOweTheGovernment(float taxRate, float salary)
    {
        this.ArbitraryTaxRate = taxRate;
        this.Salary = salary;

        CalculateAmount(this.Salary, this.ArbitraryTaxRate);
    }

    private void CalculateAmount(float salary, float taxRate)
    {
        this.AmountToPay = (salary * (taxRate / 100));
    }
}


In my contrived example I think the first is clearer, but as a class grows in size and complexity it would make it harder to track what is coming from where.

Solution

Don't store what you can calculate:

public class YouOweTheGovernment
{
    public float AmountToPay
    {
        get { return Salary * (ArbitraryTaxRate / 100); }
    }
    public float ArbitraryTaxRate { get; private set; }
    public float Salary { get; private set; }

    public YouOweTheGovernment(float taxRate, float salary)
    {
        this.ArbitraryTaxRate = taxRate;
        this.Salary = salary;
    }
}


Your AmountToPay field is basically a cache, and caching is notoriously problematic. Case in point: even your tiny code here has a cache invalidation bug. If you change the tax rate or salary, you aren't recalculating the amount to pay.

Every time you add a field, ask yourself "does this field store unique information that no other field stores?" If the answer is no, don't create the field. The less state you have, the easier it is to understand your code.

Code Snippets

public class YouOweTheGovernment
{
    public float AmountToPay
    {
        get { return Salary * (ArbitraryTaxRate / 100); }
    }
    public float ArbitraryTaxRate { get; private set; }
    public float Salary { get; private set; }

    public YouOweTheGovernment(float taxRate, float salary)
    {
        this.ArbitraryTaxRate = taxRate;
        this.Salary = salary;
    }
}

Context

StackExchange Code Review Q#362, answer score: 9

Revisions (0)

No revisions yet.