7
\$\begingroup\$

I have a class that contains 2 properties of the same type. decimal NetAmount and decimal GrossAmount

I would like to initialize it using either GrossAmount or NetAmount and based on the one specified calculate the second one.

Which way is the most elegant and why? (parameter validation is omitted for brevity)

1

public class TaxedPrice
{
    private TaxedPrice()
    { }

    public decimal NetAmount { get; private set; }
    public decimal GrossAmount { get; private set; }

    public static TaxedPrice FromNet(decimal netAmount, decimal taxRate)
    {
        return new TaxedPrice
        {
            NetAmount = decimal.Round(netAmount, 2, MidpointRounding.AwayFromZero),
            GrossAmount = decimal.Round(netAmount.ApplyTax(taxRate), 2, MidpointRounding.AwayFromZero)
        };
    }

    public static TaxedPrice FromGross(decimal grossAmount, decimal taxRate)
    {
        return new TaxedPrice
        {
            GrossAmount = decimal.Round(grossAmount, 2, MidpointRounding.AwayFromZero),
            NetAmount = decimal.Round(grossAmount.RemoveTax(taxRate), 2, MidpointRounding.AwayFromZero)
        };
    }
}

2

public class TaxedPrice
{
    public TaxedPrice(decimal netAmount, decimal grossAmount, decimal taxRate)
    {
        if (grossAmount != default)
        {
            GrossAmount = decimal.Round(grossAmount, 2, MidpointRounding.AwayFromZero);
            NetAmount = decimal.Round(grossAmount.RemoveTax(taxRate), 2, MidpointRounding.AwayFromZero);
        }
        else if (netAmount != default)
        {
            NetAmount = decimal.Round(netAmount, 2, MidpointRounding.AwayFromZero);
            GrossAmount = decimal.Round(netAmount.ApplyTax(taxRate), 2, MidpointRounding.AwayFromZero);
        }
        else
        {
            throw new InvalidOperationException($"Either {nameof(netAmount)} or {grossAmount} must be set.");
        }
    }

    public decimal NetAmount { get; }
    public decimal GrossAmount { get; }
}

3

public class TaxedPrice
{
    public enum Type
    {
        Gross,
        Net
    }

    public TaxedPrice(decimal amount, Type type, decimal taxRate)
    {
        if (type == Type.Gross)
        {
            GrossAmount = decimal.Round(amount, 2, MidpointRounding.AwayFromZero);
            NetAmount = decimal.Round(amount.RemoveTax(taxRate), 2, MidpointRounding.AwayFromZero);
        }
        else if (type == Type.Net)
        {
            NetAmount = decimal.Round(amount, 2, MidpointRounding.AwayFromZero);
            GrossAmount = decimal.Round(amount.ApplyTax(taxRate), 2, MidpointRounding.AwayFromZero);
        }
    }

    public decimal NetAmount { get; }
    public decimal GrossAmount { get; }
}

4

public class TaxedPrice
{
    public TaxedPrice(decimal amount, bool fromGross, decimal taxRate)
    {
        if (fromGross)
        {
            GrossAmount = decimal.Round(amount, 2, MidpointRounding.AwayFromZero);
            NetAmount = decimal.Round(amount.RemoveTax(taxRate), 2, MidpointRounding.AwayFromZero);
        }
        else
        {
            NetAmount = decimal.Round(amount, 2, MidpointRounding.AwayFromZero);
            GrossAmount = decimal.Round(amount.ApplyTax(taxRate), 2, MidpointRounding.AwayFromZero);
        }
    }

    public decimal NetAmount { get; }
    public decimal GrossAmount { get; }
}

How it looks like from caller's side:

// 1
var taxedPrice = TaxedPrice.FromNet(2.123m, 0.23m);
// 2
var taxedPrice = new TaxedPrice(2.123m, default, 0.23m); // uses the first one to calculate the second one
var taxedPrice2 = new TaxedPrice(2.123m, 1.11m, 0.23m); // uses the first one to calculate the second one
var taxedPrice3 = new TaxedPrice(default, 1.11m, 0.23m); // uses the second one to calculate the first one
// 3
var taxedPrice = new TaxedPrice(2.123m, TaxedPrice.Type.Net, 0.23m);
// 4 
var taxedPrice = new TaxedPrice(2.123m, false, 0.23m);

Extensions for tax:

public static class TaxExtensions
{
    public static decimal ApplyTax(this decimal netPrice, decimal taxRate)
    {
        return netPrice * (taxRate + 1);
    }

    public static decimal RemoveTax(this decimal grossPrice, decimal taxRate)
    {
        return grossPrice / (taxRate + 1);
    }
}

Mapping perspective

In my upper layer I pass those prices in POCOs/DTOs like:

public class PriceDTO
{
    public decimal NetAmount { get; set; }
    public decimal GrossAmount { get; set; }
}

And I have to check there as well which one was passed to decide from which to calculate. So in case of 1 mapping would look like:

if (priceDto.GrossAmount != default)
    return TaxedPrice.FromGross(priceDto.GrossAmount, taxRate);
else if (priceDto.NetAmount != default)
    return TaxedPrice.FromNet(priceDto.NetAmount, taxRate);
else
    // error

In case of 2 (no need to check in the mapping code)

return new TaxedPrice(priceDto.NetAmount, priceDto.GrossAmount, taxRate)

3 - there's a check as well

4 - same like 1 and 3

And I agree this could be a struct instead.

\$\endgroup\$
  • \$\begingroup\$ There are some methods missing: RemoveTax and ApplyTax. Are these methods extensions? \$\endgroup\$ – t3chb0t Oct 3 '18 at 7:56
  • \$\begingroup\$ @t3chb0t simple extensions for decimal type that add/remove tax from the price. \$\endgroup\$ – Konrad Oct 3 '18 at 7:57
  • \$\begingroup\$ ok, it'd be great if you could add them too so that people can copy/paste your code into an IDE and test it if they like ;-) \$\endgroup\$ – t3chb0t Oct 3 '18 at 8:00
  • 1
    \$\begingroup\$ @t3chb0t added code \$\endgroup\$ – Konrad Oct 3 '18 at 8:03
  • 3
    \$\begingroup\$ Now the first answer has been posted, please refrain from further editing the question. You're at risk of invalidating the existing answer, which is simply not done on Code Review. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mast Oct 3 '18 at 11:00
4
\$\begingroup\$

I'd personally go for option one as it's by far the simplest, as the names FromNet and FromGross give a clear indication of what the code is doing. That clarity is lost in the other examples.

However, there is room to improve things. Both methods perform the same basic calculation, resulting in four near-dentical expressions. So DRY can be applied here:

public class TaxedPrice
{
    private TaxedPrice()
    { }

    public decimal NetAmount { get; private set; }
    public decimal GrossAmount { get; private set; }

    public static TaxedPrice FromNet(decimal netAmount, decimal taxRate)
    {
        return new TaxedPrice
        {
            NetAmount = ApplyRounding(netAmount),
            GrossAmount = ApplyRounding(netAmount.ApplyTax(taxRate))
        };
    }

    public static TaxedPrice FromGross(decimal grossAmount, decimal taxRate)
    {
        return new TaxedPrice
        {
            GrossAmount = ApplyRounding(grossAmount),
            NetAmount = ApplyRounding(grossAmount.RemoveTax(taxRate))
        };
    }

    private decimal ApplyRounding(decimal rawValue)
        => decimal.Round(rawValue, 2, MidpointRounding.AwayFromZero)

}
\$\endgroup\$
  • \$\begingroup\$ To follow OP's pattern you could also make ApplyRounding an extension method. \$\endgroup\$ – MaLiN2223 Oct 3 '18 at 13:14
  • \$\begingroup\$ @MaLiN2223, if that same rounding formula is used throughout the code, then yes, I'd certainly do that. \$\endgroup\$ – David Arno Oct 3 '18 at 13:20
  • \$\begingroup\$ @MaLiN2223 in my project I actually have an extension method for Round so I agree \$\endgroup\$ – Konrad Oct 3 '18 at 13:20
5
\$\begingroup\$

In my opinion, you should go for number 1 (with one small change).

Reasons for choosing this one:

  1. Easy to modify - you can add new FromX method
  2. Clear - it states exactly what do you calculate
  3. Doesn't require explanation beforehand to do some checking by consumer (as in number 2)

Drawback:

(big one) It might disrupt testing, mocking a static method is a nightmare (unless you can afford moles).

Taking this all into account, I'd do a little bit of overengeneering and create a factory(-ish) class to build you a TaxedPrice object. For example:

public class TaxedPriceFactory: ITaxedPriceFactory {
    public TaxedPrice CreateFromNet(decimal netAmount, decimal taxRate) { 
    //body
    }

    public TaxedPrice CreateFromGross(decimal grossAmount, decimal taxRate){ 
    // body
    }
}

This should have all three of advantages I mentioned earlier and it is easy to use in tests now.

Addendum: Consider making your TaxedPrice a immutable struct instead of a class with two read-only properties.

EDIT: class and method names changed as suggested by @t3chb0t

\$\endgroup\$
  • 1
    \$\begingroup\$ I don't think this is overengineered. If you named it TaxedPriceFactory and CreateFromNet it'd be exactly as books suggest it ;-) \$\endgroup\$ – t3chb0t Oct 3 '18 at 8:40
  • 1
    \$\begingroup\$ I agree that option 1 is the best option. Your comments about mocking statics is nonsense though. Both methods are deterministic and thus do not need mocking. Introducing a factory is therefore pure over-engineering in this case. \$\endgroup\$ – David Arno Oct 3 '18 at 9:27
  • \$\begingroup\$ @DavidArno I disagree, you wouldn't test the TaxPrice class, you might need to test a class that depends on it. In static scenario you would need to figure out how calculate a specific output value, in mine you would just mock the output. \$\endgroup\$ – MaLiN2223 Oct 3 '18 at 12:20
  • \$\begingroup\$ "you wouldn't test the TaxPrice class". Quite the contrary: I'd definitely test that class. "In static scenario you would need to figure out how calculate a specific output value...". No figuring out required. As TaxPrice is deterministic, it can be used to provide the reference values. That's the beauty of deterministic code: no mocking required. \$\endgroup\$ – David Arno Oct 3 '18 at 12:57
  • 1
    \$\begingroup\$ @DavidArno yes, I misspoke let me rephrase: The problem with static method is not about testing this specific class. About your other sentence: how would you go about a test to see how your UI reacts to a Gross value of 0.35163453 ? You would need to input the correct values for the static method to get 0.35163453, isn't that right? \$\endgroup\$ – MaLiN2223 Oct 3 '18 at 13:01
2
\$\begingroup\$

Another possible option. Make NetAmount and GrossAmount first-class types in your system and provide implicit conversions. The only problem I'm having is how to specify a tax rate each call like the original (mine is somewhat global as you'll see by the usage example at the end). If someone has a good workaround on that front, I'd love to see it. You can also add equality/comparison methods and operators and have it implement as many interfaces (like decimal does) as you like.

NetAmount.cs

public struct NetAmount
{
    private static decimal _globalTaxRate;

    public NetAmount(decimal amount, decimal globalTaxRate)
    {
        this.Amount = decimal.Round(amount, 2, MidpointRounding.AwayFromZero);
        _globalTaxRate = globalTaxRate;
    }

    public decimal Amount { get; }

    public static decimal GlobalTaxRate => _globalTaxRate;

    public static implicit operator decimal(NetAmount netAmount)
    {
        return netAmount.Amount;
    }

    public static implicit operator NetAmount(decimal amount)
    {
        return new NetAmount(amount, _globalTaxRate);
    }

    public static implicit operator GrossAmount(NetAmount netAmount)
    {
        return decimal.Round(netAmount.ApplyTax(_globalTaxRate), 2, MidpointRounding.AwayFromZero);
    }

    public override string ToString()
    {
        return this.Amount.ToString();
    }
}

GrossAmount.cs

public struct GrossAmount
{
    private static decimal _globalTaxRate;

    public GrossAmount(decimal amount, decimal globalTaxRate)
    {
        this.Amount = decimal.Round(amount, 2, MidpointRounding.AwayFromZero);
        _globalTaxRate = globalTaxRate;
    }

    public decimal Amount { get; }

    public static decimal GlobalTaxRate => _globalTaxRate;

    public static implicit operator decimal(GrossAmount grossAmount)
    {
        return grossAmount.Amount;
    }

    public static implicit operator GrossAmount(decimal amount)
    {
        return new GrossAmount(amount, _globalTaxRate);
    }

    public static implicit operator NetAmount(GrossAmount grossAmount)
    {
        return decimal.Round(grossAmount.RemoveTax(_globalTaxRate), 2, MidpointRounding.AwayFromZero);
    }

    public override string ToString()
    {
        return this.Amount.ToString();
    }
}

TaxExtensions.cs

public static class TaxExtensions
{
    public static GrossAmount ApplyTax(this NetAmount netPrice, decimal taxRate)
    {
        return netPrice * (taxRate + 1);
    }

    public static NetAmount RemoveTax(this GrossAmount grossPrice, decimal taxRate)
    {
        return grossPrice / (taxRate + 1);
    }
}

Usage

// Only to set the global tax rate;
NetAmount init1 = new NetAmount(0.00m, 0.23m);
GrossAmount init2 = new GrossAmount(0.00m, 0.23m);

GrossAmount untaxedPrice = (NetAmount)2.13m;
NetAmount taxedPrice = (GrossAmount)2.00m;

Console.WriteLine(untaxedPrice + " " + taxedPrice);
Console.ReadLine();
\$\endgroup\$
  • \$\begingroup\$ Interesting approach \$\endgroup\$ – Konrad Oct 3 '18 at 14:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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