HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

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

Submitted by: @import:stackexchange-codereview··
0
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 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 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.