5
\$\begingroup\$

I am in my first week of programming and I created a currency converter. I want to use real time exchange rates through an api when sending the result. This code itself works just fine. But, being new I am always skeptical if it's done the proper way.

I found this method working:

var client = new WebClient();
var usdToEur = client.DownloadString("http://currencies.apps.grandtrunk.net/getlatest/usd/eur");
var usdToRon = client.DownloadString("http://currencies.apps.grandtrunk.net/getlatest/usd/ron");
var eurToUsd = client.DownloadString("http://currencies.apps.grandtrunk.net/getlatest/eur/usd");
var eurToRon = client.DownloadString("http://currencies.apps.grandtrunk.net/getlatest/eur/ron");
var ronToUsd = client.DownloadString("http://currencies.apps.grandtrunk.net/getlatest/ron/usd");
var ronToEur = client.DownloadString("http://currencies.apps.grandtrunk.net/getlatest/ron/eur");

Complete Method:

static double PerformConversion(string firstCurrency, string secondCurrency, double currencyValue)
{
    var client = new WebClient();
    var usdToEur = client.DownloadString("http://currencies.apps.grandtrunk.net/getlatest/usd/eur");
    var usdToRon = client.DownloadString("http://currencies.apps.grandtrunk.net/getlatest/usd/ron");
    var eurToUsd = client.DownloadString("http://currencies.apps.grandtrunk.net/getlatest/eur/usd");
    var eurToRon = client.DownloadString("http://currencies.apps.grandtrunk.net/getlatest/eur/ron");
    var ronToUsd = client.DownloadString("http://currencies.apps.grandtrunk.net/getlatest/ron/usd");
    var ronToEur = client.DownloadString("http://currencies.apps.grandtrunk.net/getlatest/ron/eur");

    if (firstCurrency == "dollar" && secondCurrency == "euro")
    {
        return currencyValue * Convert.ToDouble(usdToEur);
    }
    if (firstCurrency == "dollar" && secondCurrency == "ron")
    {
        return currencyValue * Convert.ToDouble(usdToRon);
    }
    if (firstCurrency == "euro" && secondCurrency == "dollar")
    {
        return currencyValue * Convert.ToDouble(eurToUsd);
    }
    if (firstCurrency == "euro" && secondCurrency == "ron")
    {
        return currencyValue * Convert.ToDouble(eurToRon);
    }

    if (firstCurrency == "ron" && secondCurrency == "dollar")
    {
        return currencyValue * Convert.ToDouble(ronToUsd);
    }

    if (firstCurrency == "ron" && secondCurrency == "euro")
    {
        return currencyValue * Convert.ToDouble(ronToEur);
    }

    return currencyValue;
}
Improve this question
\$\endgroup\$
2
  • \$\begingroup\$ Is there any particular reason why do you use WebClient instead of HttpClient? \$\endgroup\$
    – Peter Csala
    Jun 6 at 7:07
  • \$\begingroup\$ No reason. I am really new into calling and using APIs. It was more like a hit or miss, this worked but I was skeptical if it is the best method. I understood now that HttpClient it’s better and will google more about it. Thanks! \$\endgroup\$
    – South
    Jun 6 at 14:11

3 Answers 3

Reset to default
12
\$\begingroup\$

Don't use double for currency

The double type can't accurately represent base-10 decimal numbers. This is fine for a lot of applications, but it is an issue for currency, where rounding errors can cause money to disappear or be created from thin air. Prefer the decimal type in these applications.

Don't make superfluous web requests

You get 6 exchange rates from the server each time you call your method, when you only need one. Just request the one exchange rate you need after resolving which currencies you are dealing with.

Hard-coded strings

You check you function parameters against hard-coded and undocumented strings.

This makes it hard to maintain, and leaves the caller guessing how to call your functions. This is made worse as your choice of strings is inconsistent:

  • "euro" is the full name of the currency
  • "ron" is the currency code for the Romanian leu
  • "dollar" is ambiguous, as many currencies are called dollar in some form. In your case, you apparently mean "US dollar"

An alternative is to provide a class with named constants for designating various currencies.

Use of deprecated WebClient

The Microsoft C# documentation for WebClient recommends using the System.Net.Http.HttpClient class instead.

Synchronous web requests

Web requests can potentially take a long time, leaving your program hanging until it gets a response from the server.

Consider providing an asynchronous API for making currency conversions.

Logic issues

You check individually the name of the first and second currency, which means you have a lot of if statements, and this number will grow quadratically with the number of currencies you support. This is a lot of repetition.

You will save a lot of effort using string interpolation instead.

Excessive instanciations

Each time your method is called, a WebClient instance is created and left to be collected by the garbage collector. You can save resources by reusing the same instance over the lifetime of your program.

Naming

PerformConversion can be simplified to Convert.

firstCurrency and secondCurrency can be ambiguous as to which way the conversion is performed. I find fromCurrency and toCurrency to be more explicit.

Example code

Here is my take on the problem, taking into account the remarks I made:

namespace currency
{
    public class Currency
    {
        public string Name { get; init; }
        public string Code { get; init; }
        public Currency(string name, string code)
        {
            Name = name;
            Code = code;
        }
        public static Currency Euro = new Currency("Euro", "eur");
        public static Currency USDollar = new Currency("US Dollar", "usd");
        public static Currency RomanianLeu = new Currency("Romanian Leu", "ron");
    }


    public static class CurrencyConverter
    {
        private const string endpoint = @"http://currencies.apps.grandtrunk.net/getlatest/{0}/{1}";
        private static HttpClient httpClient = new HttpClient();
        public static decimal Convert(decimal amount, Currency from, Currency to)
        {
            return amount * GetExchangeRate(from, to);
        }
        public static async Task<decimal> ConvertAsync(decimal amount, Currency from, Currency to)
        {
            return amount * await GetExchangeRateAsync(from, to);
        }
        public static decimal GetExchangeRate(Currency from, Currency to)
        {
            string url = string.Format(endpoint, from.Code, to.Code);
            return decimal.Parse(httpClient.GetStringAsync(url).GetAwaiter().GetResult(), NumberFormatInfo.InvariantInfo);
        }
        public static async Task<decimal> GetExchangeRateAsync(Currency from, Currency to)
        {
            string url = string.Format(endpoint, from.Code, to.Code);
            return decimal.Parse(await httpClient.GetStringAsync(url), NumberFormatInfo.InvariantInfo);
        }
    }
}
Improve this answer
\$\endgroup\$
8
\$\begingroup\$

I notice that you're using terms like "dollar" and "euro" to denote currency. This is fine, but there are questions like which dollar. Looking at the code, it uses USD, but dollar can also mean AUD for example.

Also the URL uses ISO 4217 for denoting currencies. Why do I mention this? Because if we pass in data that conforms to what the URL expects, we can reduce the code from:

static double PerformConversion(string firstCurrency, string secondCurrency, double currencyValue)
{
    var client = new WebClient();
    var usdToEur = client.DownloadString("http://currencies.apps.grandtrunk.net/getlatest/usd/eur");
    var usdToRon = client.DownloadString("http://currencies.apps.grandtrunk.net/getlatest/usd/ron");
    var eurToUsd = client.DownloadString("http://currencies.apps.grandtrunk.net/getlatest/eur/usd");
    var eurToRon = client.DownloadString("http://currencies.apps.grandtrunk.net/getlatest/eur/ron");
    var ronToUsd = client.DownloadString("http://currencies.apps.grandtrunk.net/getlatest/ron/usd");
    var ronToEur = client.DownloadString("http://currencies.apps.grandtrunk.net/getlatest/ron/eur");

    if (firstCurrency == "dollar" && secondCurrency == "euro")
    {
        return currencyValue * Convert.ToDouble(usdToEur);
    }
    if (firstCurrency == "dollar" && secondCurrency == "ron")
    {
        return currencyValue * Convert.ToDouble(usdToRon);
    }
    if (firstCurrency == "euro" && secondCurrency == "dollar")
    {
        return currencyValue * Convert.ToDouble(eurToUsd);
    }
    if (firstCurrency == "euro" && secondCurrency == "ron")
    {
        return currencyValue * Convert.ToDouble(eurToRon);
    }

    if (firstCurrency == "ron" && secondCurrency == "dollar")
    {
        return currencyValue * Convert.ToDouble(ronToUsd);
    }

    if (firstCurrency == "ron" && secondCurrency == "euro")
    {
        return currencyValue * Convert.ToDouble(ronToEur);
    }

    return currencyValue;
}

To

static double PerformConversion(string firstCurrency, string secondCurrency, double currencyValue)
{
    var client = new WebClient();
    var conversionRateResponse = client.DownloadString($"http://currencies.apps.grandtrunk.net/getlatest/{firstCurrency}/{secondCurrency}");

    return currencyValue * Convert.ToDouble(conversionRateResponse);
}

Also, WebClient is deprecated (prefer to use HttpClient instead), however it should be fine for your use case.

Improve this answer
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Yes, good advice in general... if there are good standard codes already in existence (e.g. currencies, countries, airports, etc), use them instead of making up your own. There's rarely any benefit in doing your own thing in that respect, and you avoid a lot of unnecessary and error-prone conversions whenever you have to communicate with others. \$\endgroup\$
    – Simon Geard
    Jun 6 at 22:42
3
\$\begingroup\$

  • Rather than if A & B, consider a nested if. This avoided repeat tests

  • Tolerate conversion to the same unit. $ to $ should be OK.

  • Detect unexpected currencies. Be skeptical of user input.

  • Find the conversion factor, then multiply

    double conversionFactor = 1.0;  // Default to convert into same units
    if (firstCurrency != secondCurrency) {
      if (firstCurrency == "dollar") {
        if (secondCurrency == "euro") {
          conversionFactor = Convert.ToDouble(usdToEur);
        } else if (secondCurrency == "ron") {
          conversionFactor = Convert.ToDouble(usdToRon);
        } else {
          Handle error with TBD code
        }
      } else if (firstCurrency == "euro") {
        if (secondCurrency == "dollar") {
          conversionFactor = Convert.ToDouble(eurToUsd);
        } else if (secondCurrency == "ron") {
          conversionFactor = Convert.ToDouble(eurToRon);
        } else {
          Handle error with TBD code
        }
      } else if (firstCurrency == "ron") {
        if (secondCurrency == "dollar") {
          conversionFactor = Convert.ToDouble(ronToUsd);
        } else if (secondCurrency == "euro") {
          conversionFactor = Convert.ToDouble(ronToEur);
        } else if (secondCurrency != "ron") { 
          Handle error with TBD code
        }
      } else {
        Handle error with TBD code
      }
    }
    return currencyValue * converiosnFactor;

Next, consider 3x3 look-up table and code like:

Validate and then convert 1st currency type into index1.  
Validate and then convert 2nd currency type into index2.   
conversion factor = table[table1][table2];
return currencyValue * converiosnFactor;

Code downloads 6 conversions with var ... = client.DownloadString(...); yet only, at most, 1 is needed. Perhaps only call client.DownloadString() for the 1 conversion needed.

Improve this answer
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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