patterncsharpMinor
"Do you owe the government?" classes
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?
Or explicitly pass parameters into a procedure?
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.
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:
Your
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.
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.