Value formatters (for entities like price, currencies so on)

Question

I am looking over some existing code (see below). The problem of grouping in the same place the presentation logic of some fields (like price, currency) was solved as extension methods on int, double, and so on...

I do not really like this. What do you think? How could it be done more elegantly and even unit testable?

namespace RestApi.Services.Helpers
{
    using System;
    using System.Globalization;
    using System.Web.Mvc;
    using Contracts.Fg;
    using FrontEnd.CommonServices.Localization;

    public enum QuoteDisplayType
    {
        BidAsk,
        Difference,
        Other
    }

    public static class CommonFormatterExtensions
    {
        private static ILocalizationService localizationService;

        private static ILocalizationService LocalizationService
        {
            get { return localizationService ?? (localizationService = DependencyResolver.Current.GetService<ILocalizationService>()); }
        }


        private const string NoInformationText = "---";
        private const string MarketLocalizationKey = "FormatAmountExtensions.MarketQuote";

        private const string PercentageSymbol = "%";

        private const string CurrencyCodeEuro = "EUR";
        private const string CurrencyCodeUsDollar = "USD";
        private const string CurrencyCodeBritishPound = "GBP";
        private const string CurrencyCodeDutchGuilder = "NLG";

        private const string SymbolEuro = "€";
        private const string SymbolDollar = "$";
        private const string SymbolDutchGuilders = "fl";
        private const string SymbolBritishPound = "£";

        private const string FormatWithMinTwoDecimals = "#,##0.00###";
        private const string FormatWithAllTrailingDecimalZeroesSupressed = "#,##0.#####";


        /// <summary>
        /// Returns a formatted number for securities. When no fractions (decimals) are present, the number is formatted without decimals. 
        /// Note that altough we (topline) currrently do not support fractions (decimals) in security holdings, we do receive the number as a 'double' from Topline, and therefore also support the formatting of fractions.
        /// </summary>
        /// <param name="amount">amount to be formatted</param>
        /// <returns>a string represented the formatted number</returns>
        public static string FormattedNumberOfSecurities(this double amount)
        {
            return amount.Equals(0)
                       ? NoInformationText
                       : amount.ToString(FormatWithAllTrailingDecimalZeroesSupressed, CultureInfo.CurrentCulture);
        }

        public static string FormattedNumberOfSecurities(this double? amount)
        {
            return amount.HasValue ? amount.Value.FormattedNumberOfSecurities() : NoInformationText;
        }

        public static string FormattedNumberOfSecurities(this int amount)
        {
            return amount.Equals(0)
                        ? NoInformationText
                        : amount.ToString(FormatWithAllTrailingDecimalZeroesSupressed, CultureInfo.CurrentCulture);
        }

        public static string FormattedNumberOfSecurities(this int? amount)
        {
            return amount.HasValue ? amount.Value.FormattedNumberOfSecurities() : NoInformationText;
        }


        /// <summary>
        /// Returns a formatted price for a security. Result has at least 2 decimals and max 5 decimals. 
        /// </summary>
        /// <param name="amount">amount (price) to be formatted</param>
        /// <param name="mainsecurityType">main security type of the security for which the price is to be formatted</param>
        /// <param name="amountCurrencyCode">currency in which the security is traded</param>
        /// <param name="quoteDisplayType">whether or not it is a bid/ask quote</param>
        /// <returns>a string represented the formatted price</returns>
        public static string FormattedSecurityPrice(this double amount, Hoofdfondstype mainsecurityType, string amountCurrencyCode, QuoteDisplayType quoteDisplayType = QuoteDisplayType.Other)
        {
            string formattedAmount;

            if (amount.Equals(0))
            {
                //should only be applicable to Bid/Aks quotes.
                formattedAmount = quoteDisplayType == QuoteDisplayType.BidAsk ? LocalizationService.Localize(MarketLocalizationKey) : NoInformationText;
            }
            else
            {
                formattedAmount = amount.ToString(FormatWithMinTwoDecimals, CultureInfo.CurrentCulture);

                if (amountCurrencyCode != CurrencyCodeEuro && mainsecurityType != Hoofdfondstype.Indices && quoteDisplayType != QuoteDisplayType.Difference)
                // Security prices and quotes are always shown with a currency indication, except when in EURO or when security is an index.
                // A price difference is never displayed with a currency indication.
                {
                    formattedAmount = AddCurrencyIndication(amountCurrencyCode, formattedAmount);
                }

                if ((quoteDisplayType != QuoteDisplayType.Difference) &&
                    (mainsecurityType == Hoofdfondstype.Coupons || mainsecurityType == Hoofdfondstype.Obligatie ||
                    mainsecurityType == Hoofdfondstype.ObligatieEmissies))
                // Price of coupons and bonds is displayed as percentage, except when the price reflects a difference. 
                {
                    formattedAmount = String.Format(CultureInfo.CurrentCulture, "{0} {1}", formattedAmount,
                                                    PercentageSymbol);
                }
            }
            return formattedAmount;
        }

        /// <summary>
        /// Returns a formatted price for a security. Result has at least 2 decimals and max 5 decimals. 
        /// </summary>
        /// <param name="amount">nullable amount (price) to be formatted</param>
        /// <param name="mainsecurityType">main security type of the security for which the price is to be formatted</param>
        /// <param name="amountCurrencyCode">currency in which the security is traded</param>
        /// <returns>a string represented the formatted price or three dashes if no value is available</returns>
        public static string FormattedSecurityPrice(this double? amount, Hoofdfondstype mainsecurityType, string amountCurrencyCode)
        {
            return amount.HasValue
                ? amount.Value.FormattedSecurityPrice(mainsecurityType, amountCurrencyCode)
                : NoInformationText;
        }


        private static string AddCurrencyIndication(string amountCurrencyCode, string formattedAmount)
        {
            switch (amountCurrencyCode)
            {
                case CurrencyCodeUsDollar:
                    formattedAmount = SymbolDollar + " " + formattedAmount;
                    break;
                case CurrencyCodeEuro:
                    formattedAmount = SymbolEuro + " " + formattedAmount;
                    break;
                case CurrencyCodeDutchGuilder:
                    formattedAmount = SymbolDutchGuilders + " " + formattedAmount;
                    break;
                case CurrencyCodeBritishPound:
                    formattedAmount = SymbolBritishPound + " " + formattedAmount;
                    break;
                default:
                    formattedAmount = amountCurrencyCode + " " + formattedAmount;
                    break;
            }
            return formattedAmount;
        }
    }


}

Answer 1

The big switch statement in AddCurrencyIndication is a pretty good sign of where to start. If you had to add another type of currency, you'd have to go in and add new cases, which violates the open/close principle. There's a lot of places where you either do already, or potentially in the future, have to go in and add new items or logic when a new currency is added. So what you probably want is a formatter class for each currency: EuroFormatter, UsDollarFormatter, etc.

You would probably have an interface ICurrencyFormatter with at least FormattedSecurityPrice as a non-static public member on it.

It looks like you have some logic that is common, so you can put that in an abstract base class. If it's something that might change for specific currencies (I notice the check for if the currency is Euros inside FormattedSecurityPrice for example) then you can make it virtual and override where needed in the specific class.

Your const strings can be replaced with overrideable properties, so for example instead of

private const string SymbolEuro = "€";

You'd have in CurrencyFormatterBase:

protected abstract string Symbol { get; }

And inside EuroFormatter:

protected override string Symbol { get { return "€"; } }

Depending on how they're used, it's possible that some of your methods, like FormattedNumberOfSecurities might be appropriate to keep as extension methods. But really most of those look like they should probably be regular static or instance methods.

You also have a bit of repetition within the FormattedNumberOfSecurities overloads. You might consider pulling some of the common logic out into a private method. For example you could convert whatever type is passed in to string, then pass that string along to a private method that deals with the formatting and handling of nulls.

A final thing to investigate would be whether Hoofdfondstype and QuoteDisplayType could be refactored into classes with their own functionality which could be called out to rather than having to do complex logic based on which one you have. But that's a more wide-ranging change and I couldn't really recommend whether or not it's a good idea in this particular situation without seeing a lot more of the project's code.

Answer 2

I like the use of comments in this code snippet. They are explaining why a certain piece of code is done a certain way, rather than what it is doing.

I'm not sure about using extension methods for this functionality, it kind of feels like its abusing the feature.

I would have probably made an interface IFormatter, which could then be injected as a singleton from an IoC container as required. With the CurrencyInformation class I mention below, it is possible to add currencies without ever having to touch this code again.

For starters, I would move the currency information into its own class. As part of that I would pass:

public class CurrencyInformation
{
    public string CurrencyCode { get; private set; }
    public string Symbol { get; private set; }

    public CurrencyInformation(string currencyCode, string symbol)
    {
         // Null and empty string checks
         // assign to properties
    }

    public string Format(string formattedAmount)
    {
        return string.Format("{0} {1}", CurrencyCode, formattedAmount);
    }
}

This will eliminate a number of constants from the extension class and replace them with a simple IDictionary. This makes it much easier to add more currencies in the future.

private static IDictionary<string, CurrencyInformation> Currencies = new Dictionary<string, CurrencyInformation>
                           {
                                {"EUR", new CurrencyInformation("EUR", "€")},
                                // fill in rest of currencies
                           }

The AddCurrencyIndication method now looks like this

  private static string AddCurrencyIndication(string amountCurrencyCode, string formattedAmount)
  {

       if (!Currencies.ContainsKey(amountCurrencyCode)) return string.Format("{0} {1}", amountCurrencyCode, formattedAmount);

       return Currencies[amountCurrencyCode].Format(formattedAmount);
  }

Having this class also allows the formatting to be different for different currencies (i.e. using "," instead of "." to separate dollars and cents

If you do move this class to not be extension methods, you could actually inject all the information from a config file, so you'd never have to touch any of these classes, but this technique is a little beyond this review.

I like that you've used ternary operations for your formatting functions, it makes things easy to read. I would suggest fixing the formatting so they are all formatted the same. This way, people who are reading it in the future don't have to keep changing their brain between the two. The format I like is:

return <condition>
       ? <true>
       : <false>;

My last item is in FormattedSecurityPrice. This is the only method you've used an if/else in the entire class. I would change this to either exit early (add a return in the if part, and remove the else) or change it to a ternary operator like the rest of the class. The else part could be moved to its own method to allow for this to happen.