patterncsharpModerate
Value formatters (for entities like price, currencies so on)
Viewed 0 times
pricelikevalueforentitiesformatterscurrencies
Problem
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
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()); }
}
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.#####";
///
/// 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 sec
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()); }
}
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.#####";
///
/// 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 sec
Solution
The big switch statement in
You would probably have an interface
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
Your const strings can be replaced with overrideable properties, so for example instead of
You'd have in
And inside
Depending on how they're used, it's possible that some of your methods, like
You also have a bit of repetition within the
A final thing to investigate would be whether
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.Code Snippets
private const string SymbolEuro = "€";protected abstract string Symbol { get; }protected override string Symbol { get { return "€"; } }Context
StackExchange Code Review Q#43309, answer score: 10
Revisions (0)
No revisions yet.