Exchange rate calculator using XML

Here is my exchange rate calculator, i took xml file from here:
xml_rates

I would like a review if I am doing everything right, and what I could do better. I am not sure if I should make another class for all of these methods? Shall I also create class for loading xml file? Thank you guys for looking at it.

```
public static void Main(string[] args)
{
string currencyToChange = "", actualCurrency= "";
decimal money = 0;

// Reading input from User and checking if currency codes have proper length
while( input(ref actualCurrency, ref currencyToChange, ref money)==false)
{
Console.WriteLine("Currency code is not 3 letter long try again");
}

XmlDocument nbpXml = new XmlDocument();

// Loading xml
try
{
nbpXml.Load("http://www.nbp.pl/kursy/xml/a240z161213.xml");
}
catch (XmlException)
{
Console.WriteLine("Cannot load Xml file");
}
if( check( nbpXml, currencyToChange, money, actualCurrency)==false)
{
Console.WriteLine(" Could not find currency code");
}

}
// changing string to decimal
static decimal strtodec(string value)
{

decimal decimalVal = 0;

try
{
decimalVal = System.Convert.ToDecimal(value);
}
catch (System.OverflowException)
{
System.Console.WriteLine(
"The conversion from string to decimal overflowed.");
}
catch (System.FormatException)
{
System.Console.WriteLine(
"The string is not formatted as a decimal.");
}
catch (System.ArgumentNullException)
{
System.Console.WriteLine( "The string is null.");
}

return decimalVal;
}
//exchanging PLN to any currency
static decimal exchangePLN(decimal x, decimal y)
{

if ((x != 0) && (y != 0))
{
x = 1 / y * x;
}

return x;
}
//exchanging any currency to any currency
static decimal exchangeOther(decimal x, decimal y)
{
x = x / y*100;
return x;
}
//reading input
s

Just a few general remarks...

It's much easier to work with the newer XDocument then with the XmlDocument because you can use LINQ to query the XML


I am not sure if I should make another class for all of these methods?

Yes, you should encapsulate the currency logic in a dedicated class.

check( nbpXml, currencyToChange, money, actualCurrency)

Use names that indicate what the method is doing. check is too general. The method name should tell you what you are checking.

catch (XmlException)
    {
        Console.WriteLine("Cannot load Xml file");
    }

This isn't really helpful as there is no information about what went wrong. Use at least the ex.Message or for logging purposes ex.ToString().

catch (XmlException ex)

strtodec

Use descriptive names like ConvertStringToDecimal or just ConvertToDecimal or even better ParseCurrency.

decimalVal = System.Convert.ToDecimal(value);

Rather then this and catching exceptions use decimal.TryParse. This won't throw and is much faster. You should avoid exeptions where possible.

static bool input(ref string x, ref string y, ref decimal z)

x, y, z... naming! ;-)

if ((x.Length != 3) || (y.Length != 3) || (x==y))

Use helper variables that describe the condition.

var isValidCurrencyCode = ...

or

var isValidCurrencyInput = ...

or anything with a nice name.

Let me show you a small example how you could start optimizing your application.

The most important thing is to always put everything in a dedicated classes.

First you need to get the currency rates from the url so the ExchangeRateDownloader will do this job:

class ExchangeRateDownloader
{
    private readonly string _url;

    public ExchangeRateDownloader(string url)
    {
        _url = url;
    }

    public XDocument DownloadExchangeRates()
    {
        return XDocument.Load(_url);
    }
}

I uses the XDocument to get the data.

After you downloaded the XML you need to parse it. For this you need to write another class, the ExchangeRateParser.

class ExchangeRateParser
{ 
    public IEnumerable ParseExchangeRates(XDocument xRates)
    {
        return xRates.Root.Elements("pozycja").Select(x => new ExchangeRate(x));
    }
}

I delegated parsing the elements to another class. The data class.

class ExchangeRate
{ 
    public ExchangeRate(XElement xRate)
    {
        CurrencyName = xRate.Element("nazwa_waluty").Value;
        ConversionRate = decimal.Parse(xRate.Element("przelicznik").Value);
        CurrencyCode = xRate.Element("kod_waluty").Value;
        AverageRate = decimal.Parse(xRate.Element("kurs_sredni").Value);
    }

    public string CurrencyName { get; }

    public decimal ConversionRate { get; }

    public string CurrencyCode { get; }

    public decimal AverageRate { get; }
}

The last class is the CurrencyConverter. It requires a list of exchange rates.

class CurrencyConverter
{ 
    private readonly IDictionary _exchangeRates;
    public CurrencyConverter(IList exchangeRates)
    {
        _exchangeRates = exchangeRates.ToDictionary(x => x.CurrencyCode, StringComparer.OrdinalIgnoreCase);
    }

    public decimal Convert(string fromCurrencyCode, string toCurrencyCode, decimal value)
    {
        var fromCurrency = _exchangeRates[fromCurrencyCode];
        var toCurrency = _exchangeRates[toCurrencyCode];
        var pln = fromCurrency.AverageRate * value;
        if (toCurrency.CurrencyCode.Equals("PLN", StringComparison.OrdinalIgnoreCase)) 
        {
            return pln;
        }
        return pln / toCurrency.AverageRate;
    }
}

This class imports the exchange rates into a dictionary for faster and easier lookup.

The dictionary ignores the casing with by using the StringComparer.OrdinalIgnoreCase argument so you can enter any name.

You use it by specifying two currency codes and a value that you want to convert.

If the target currency is PLN then it returns the intermediate value. Each value needs to be first converted into PLN and then into the other currency.

This examples lacks a lot of error checks. For example what if a currency code does not exist or is too short or too long? It'll crash. To prevent it you can use the TryGetValue method of the dictionary.

You'll still need to add one more class for reading from the console. Yes, you should encapsulate this too.

By building it this way you can now easily write tests for each unit. Additionaly you can add another constructor to the ExchangeRate class so that you don't need a XML in tests.

Example usage:

```
var exchangeRateDownloader = new ExchangeRateDownloader("http://www.nbp.pl/kursy/xml/a240z161213.xml");
var exchangeRateParser = new ExchangeRateParser();

var xRates = exchangeRateDownloader.DownloadExchangeRates();
var exchangeRates = exchangeRateParser.ParseExchangeRates(xRates).ToList();
var currencyConverter = new CurrencyConverter(exchangeRates);

// read curre

StackExchange Code Review Q#150006, answer score: 3