4

I work on an application which, in part, calculates tax bill amounts. A tax bill is comprised of many calculable fields (sheriff fees, clerk fees, penalties, interest, flat rates, etc.) whose mode of calculation is usually static, but may change due to legislation or special bill properties. Whole categories of calculations may also be removed or added over time.

To push the clutter of branching logic away from the client, I wrote a factory for each calculation category that will apply the proper calculation given the bill's information, like so (CalcType is an enum):

var bill = new Bill(){year = 2013};
bill.AdvertisingFee = CalculationFactory.GetFee(CalcType.AdvFee, bill);

That much is simple, but I'm troubled by the way some of my concrete classes will be implemented. Here is the calculation interface:

public interface ITaxCalculation{
    decimal Calculate();
}

A typical implementation will have some sort of calculation or data access, but certain years/bill properties yield no advertising fee, like so:

public class FinanceCabinetAdvertisingFee : ITaxCalculation
{
    public decimal Calculate()
    {
        return 0.00M;
    }
}

This sort of stub class will be present for many, but not all calculation categories, for various reasons (the calculation didn't exist for the tax year, the state bought the bill, etc.)

My question: Are logic-free classes like this considered a code smell, or just an ugly fact of modeling certain unstable, real-world systems? I like the idea of having these cases firmly documented in a class rather than as the default return value of some control structure, but I'm open to the idea that I'm applying the wrong sort of pattern to this style of problem. Any additional thoughts are welcome.

abarger
  • 599
  • 7
  • 21
  • 5
    Are they logic-free? I'd say a fee of zero is logic too. Adding a property `YieldsFee` wouldn't add much unless you want to distinguish a fee of 0 from "no fee". But even in that case, returning a `decimal?` is also an option. – C.Evenhuis Feb 21 '14 at 23:03
  • 1
    @FolksymAndrew I would definitely say that there's a big difference between having `0.00` to pay and not having to pay at all. In such cases I prefer to use nullable values rather than having such, maybe obvious conventions, like paying `0.00` is the same as not having to pay. At some point you may really need to distinguish those cases that really were pay-free, from those that say - have already paid and currently they have to pay `0.00` but that's because they've already paid. – Leron Feb 21 '14 at 23:29
  • @Leron these are good points. After learning that IEnumerable has a Sum method for nullables, I realized that simply returning nulls for times when the fee did not exist would not be painful or clunky to handle. This still leaves a question, though - Is it preferable to alter my FinanceCabinetAdvertisingFee class so that Calculate() returns null as a form of positive documentation, or let the factory return null and let the class' non-existence serve as documentation? – abarger Feb 23 '14 at 17:11
  • @FolksymAndrew Well, if I were you I would change both. But I think it's better to listen to at least few more opinions. Every time you leave something as self explainable you leave an opportunity for someone to misinterpret your logic. – Leron Feb 23 '14 at 17:27
  • @C.Evenhuis I agree that it may be worth to separate 0 fee from no fee and that really depends on specifications. However, I'd propose to do this with Nullable<> approach, i.e. `decimal? Fee()` as this would make it more explicit and less prone to coding errors where one forgets to check `YieldsFee` value before examining `Fee`. – LB2 Feb 24 '14 at 22:05

1 Answers1

1

I would put magic values like this in a database or config file, possibly encrypted if you want to hide it from the users. Then this class would be a data access class, and it would be easier and more modular to make updates to the values when you need to.

Hardcoding data in the code is sometimes necessary, but it's probably not optimal for this.

Adam Miller
  • 767
  • 1
  • 9
  • 22