4
\$\begingroup\$

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
static bool input(ref string x, ref string y, ref decimal z)
{
    Console.WriteLine(" Hello Which courrency do you own? Give me currency code");
    x = Console.ReadLine();

    Console.WriteLine(" How much do you want to exchange?");
    y = Console.ReadLine();

    z = strtodec(y);

    Console.WriteLine(" Which currency do you want buy?");
    y = Console.ReadLine();

   x = x.ToUpper();
   y = y.ToUpper();

    if ((x.Length != 3) || (y.Length != 3) || (x==y))
    {
        return false;
    }
    return true;
}
// checking exchange rate
static bool check( XmlDocument doc, string currencyToChange, decimal money, string actualCurrency)
{
    //SELECTING NODE
    XmlNodeList itemNodes = doc.SelectNodes("//tabela_kursow/pozycja");
    decimal coursenbp = 0;
    decimal courseactucal = 0;
    // CHECKING EVERY NODE FOR MATCHING currency code
    foreach (XmlNode itemNode in itemNodes)
    {
        XmlNode currxml = itemNode.SelectSingleNode("kod_waluty");
        XmlNode coursexml;

        if (currxml.InnerText == currencyToChange)
        {
             coursexml = itemNode.SelectSingleNode("kurs_sredni");
            // PLN TO ANY CURRENCY do not need more info
            if (actualCurrency == "PLN")
            {

                Console.WriteLine(" Hello, You want to change:" + money + " " + actualCurrency + " You will get:" +
                Decimal.Round(exchangePLN(money, strtodec(coursexml.InnerText)),2, MidpointRounding.AwayFromZero) + " " + currencyToChange);
                return true;
            }
            //  to exchange other currency to any currency(beyond PLN). WE need info about two exchange rates
            else
            {
                //first one is stored in temporary variable - coursenbp 
                coursenbp = strtodec(coursexml.InnerText);
            }                                  
        }
        // here is second variable- courseactual
        else if((actualCurrency == currxml.InnerText) && (currencyToChange!="PLN"))
        {
            coursexml = itemNode.SelectSingleNode("kurs_sredni");
            courseactucal = strtodec(coursexml.InnerText);
        }
        // exception only when currencytocahnge is PLN
        else if((actualCurrency == currxml.InnerText) && (currencyToChange == "PLN"))
        {
            coursexml = itemNode.SelectSingleNode("kurs_sredni");
            courseactucal = strtodec(coursexml.InnerText);          

            Console.WriteLine(" Hello, You want to change:" + money + " " + actualCurrency + " You will get:" +
            Decimal.Round((courseactucal*money), 2, MidpointRounding.AwayFromZero) + " " + currencyToChange);

            return true;
        }
    }



    if ((courseactucal != 0) && (coursenbp != 0))
    {
        Console.WriteLine(" Hello, You want to change:" + money + " " + actualCurrency + " You will get:" + Decimal.Round(exchangeOther(courseactucal, coursenbp), 2, MidpointRounding.AwayFromZero) + " " + currencyToChange);
        return true;
    }
    return false;
}
\$\endgroup\$
3
\$\begingroup\$

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<ExchangeRate> 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<string, ExchangeRate> _exchangeRates;
    public CurrencyConverter(IList<ExchangeRate> 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 currency codes and value from console with another module

var eurInUsd = currencyConverter.Convert("eur", "usd", 20);

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

You usually don't need any ref parameters. If you have more then one of them then you most probably should create a new type for the return values.

Notice also how the new strong names now clearly explain and document what each parameter is for.

\$\endgroup\$
  • \$\begingroup\$ Thank you! That is what i was looking for. I am not sure about loading xml, shall I do it in class constructor or leave it in main and just pass it to class? \$\endgroup\$ – Kamil Leniak czarny Dec 15 '16 at 18:07
  • \$\begingroup\$ @KamilLeniakczarny I've merged the second answer into this one. \$\endgroup\$ – t3chb0t Dec 17 '16 at 23:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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