6
\$\begingroup\$

I'm just looking for someone to give a critique of my currency converter. I want to see if I could clean it up first before dummy-proofing. I know you want to avoid floating types for working with money, so I made a Cents class. I'm not fully confident in how I did it, so any pointers on that would be nice.

Converter.h

        #ifndef CURRENCY_H
        #define CURRENCY_H
        #include <map>

        class Converter {
            private:
                std::map<std::string, double> currency;
                std::map<std::string, std::string> symbol;
            public:
                Converter();
                std::string getSymbol(std::string currency);
                long convertCurrency(std::string c1, std::string c2, long amount);
        };

        #endif

Converter.cpp

        #include <iostream>
        #include "converter.h"

        Converter::Converter(){
            currency["usd-gbp"] = 0.6392;
            currency["usd-cad"] = 1.0478;
            currency["usd-eur"] = 0.7494; 
            currency["usd-jpy"] = 97.821; 
            currency["usd-aud"] = 1.1151;
            currency["usd-chf"] = 0.9232;
            symbol["usd"] = "$";

            currency["gbp-usd"] = 1.5646;
            currency["gbp-cad"] = 1.6393;
            currency["gbp-eur"] = 1.1726;
            currency["gbp-jpy"] = 153.0483;
            currency["gbp-aud"] = 1.7447;
            currency["gbp-chf"] = 1.4443;
            symbol["gbp"] = "£";

            currency["aud-usd"] = 0.8968;
            currency["aud-gbp"] = 0.5732;
            currency["aud-cad"] = 0.9396;
            currency["aud-eur"] = 0.6721;
            currency["aud-jpy"] = 87.7225;
            currency["aud-chf"] = 0.8278;
            symbol["aud"] = "$";


            currency["cad-usd"] = 0.9544;
            currency["cad-gbp"] = 0.61;
            currency["cad-aud"] = 1.4879;
            currency["cad-eur"] = 0.7153;
            currency["cad-jpy"] = 93.3629;
            currency["cad-chf"] = 0.8811;
            symbol["cad"] = "C$";

            currency["eur-usd"] = 1.3343;
            currency["eur-gbp"] = 0.8528;
            currency["eur-aud"] = 1.4879;
            currency["eur-cad"] = 1.398;
            currency["eur-jpy"] = 130.526;
            currency["eur-chf"] = 1.2318;
            symbol["eur"] = "€";

            currency["jpy-usd"] = 0.0102;
            currency["jpy-gbp"] = 0.0065;
            currency["jpy-aud"] = 0.0114;
            currency["jpy-cad"] = 0.0107;
            currency["jpy-eur"] = 0.0077;
            currency["jpy-chf"] = 0.0094;
            symbol["jpy"] = "¥";

            currency["chf-usd"] = 0.0065;
            currency["chf-gbp"] = 0.6924;
            currency["chf-aud"] = 1.2079;
            currency["chf-cad"] = 1.135;
            currency["chf-eur"] = 0.8118;
            currency["chf-jpy"] = 105.9644;
            symbol["chf"] = "CHF ";
        }

        std::string Converter::getSymbol(std::string currency){
            return symbol[currency];
        }

        long Converter::convertCurrency(std::string c1, std::string c2, long amount){
            std::string key = c1 + "-" + c2;
            for(int idx = 0; idx < currency.size(); idx++){
                if(currency.find(key) != currency.end()){
                    double conversion = currency[key]; 
                    conversion *= (double)amount;
                    return (long)conversion;
                } 
            }
        }

Cents.h

        #ifndef MONEY_H
        #define MONEY_H
        #include <iostream>
        #include <iomanip>

        class Cents {
            private:
                long outputCents;
                double inputCents;
            public:
                Cents(long nCents = 0){
                    outputCents = nCents;
                }

                long getLong(){ return outputCents; }

                friend std::ostream& operator<< (std::ostream &out, Cents &cCents){
                    out << std::fixed << std::setprecision(2) << (double)cCents.outputCents/100;
                    return out;
                }

                friend std::istream& operator>> (std::istream &in, Cents &cCents){
                    in >> cCents.inputCents;
                    cCents.inputCents *= 100;
                    cCents.inputCents += 0.5;
                    cCents.outputCents = cCents.inputCents;
                    std::cin.ignore(3, '\n');
                    return in;
                }
        };

        #endif

Main.cpp

#include <iostream>
#include "cents.h"
#include "converter.h"

using namespace std;

int main(){
    cout << "### Currency Converter ###\n";
    cout << "USD, GBP, CAD, EUR, JPY, AUD, CHF\n";

    string currency1, currency2;

    cout << "-Enter currency to convert from:\n";
    getline(cin, currency1);

    cout << "-Enter currency to convert to:\n";
    getline(cin, currency2);

    Cents amount;
    cout << "-Enter amount:\n";
    cin >> amount;

    Converter convert;
    Cents product = convert.convertCurrency(currency1, currency2, amount.getLong());
    if(product.getLong() > 0){
        cout << convert.getSymbol(currency2) << product << endl;
    } else {
        cout << "Currency not found!\n";
    }

    return 0;
}
\$\endgroup\$

1 Answer 1

7
\$\begingroup\$
  • You're not using using namespace std in your classes, which is good. However, for Main.cpp, it's best to put it inside the function instead of making it global.

  • getSymbol()'s parameter should be a const& since it's not being modified:

    std::string Converter::getSymbol(std::string const& currency) {}
    
  • getLong() should be const since it's an accessor:

    long getLong() const { return outputCents; }
    

    More importantly, it should actually be removed as it's bad for encapsulation (accessors, also mutators, expose the implementation details). operator<< should remain a friend so that it can still access private members.

    Moreover, I'd have another function modify outputCents, then have operator<< call it to obtain and display the new value. operator<< shouldn't do extra work, which is why cCents must still be passed by value here. After this change, cCents can become a const& instead, which is preferred for operator<<'s object parameter.

  • This form of casting:

    conversion *= (double)amount;
    

    is more C-like. Prefer the C++ way of casting:

    conversion *= static_cast<double>(amount);
    
  • The for-loop in convertCurrency() is redundant and should be removed. The if-statement already does what's needed to search the map. In general: loops for STL container classes, such as std::map, should use the iterators instead of indices.

  • Just a minor point: I'd put currency1 and currency2 right next to the getline()s:

    std::cout << "-Enter currency to convert from:\n";
    std::string currency1;
    getline(std::cin, currency1);
    
    std::cout << "-Enter currency to convert to:\n";
    std::string currency2;
    getline(std::cin, currency2);
    

    This is preferred as it keeps the variable's scope narrow, increasing maintenance and readability.

    I'd also rename both input strings to something like currencyFrom and currencyTo.

  • For more user-friendliness, consider an input menu. This should also require less input-validation as you won't need to rely on the inputted strings exactly matching the map's contents. The menu could be put into a free function, in case a client wants to use it. Doing so would ensure maintainability, flexibility, and main()'s purpose as the driver of your program.

    Generally-speaking, no one is ever obligated to use your own driver. Your program should work with any sensible, non-breaking driver.

\$\endgroup\$
8
  • 1
    \$\begingroup\$ Using switch instead of map is a bad idea. You are moving from a data driven system to a hard coded system. The use of the map allows you to use a data driven technique, this allows the system to be updated without modifying the code (and thus recompiling and restesting the code). All you have to do is load the data from an external source into the program rather than having hand coded map initialization. \$\endgroup\$ Aug 23, 2013 at 2:00
  • 1
    \$\begingroup\$ Also disagree on your comments about the output operator. Note it is not a member as defined (as friends are not members). Rather than make the output operator not a friend and use an accessor. Prefer to keep it a friend and remove the accessor. get/set methods break encapsulation be exposing implementation details. \$\endgroup\$ Aug 23, 2013 at 2:03
  • 1
    \$\begingroup\$ The comment about using std::size in the loop in currencyConverter() is also not a good idea. When iterating over a container you should use the iterators. But also not that you missed the fact that the loop is completely redundant and should be removed. \$\endgroup\$ Aug 23, 2013 at 2:06
  • \$\begingroup\$ @LokiAstari: Thanks. I wasn't paying enough attention to the loop, though I figured std::size_t would've been good enough. As for the map, I didn't think about it that way. It did seem unneeded at a glance. I'll make the changes. \$\endgroup\$
    – Jamal
    Aug 23, 2013 at 2:12
  • \$\begingroup\$ Thank you both for the input, I've made the changes suggested. I'm just curious why the accessor should be removed. How is it that it exposes implementation details and why is this bad? I'm also unsure of how to do an alternative to calling the accessor. To summarize, what I'm doing in main is inputting two strings, inputting the monetary amount into a Cents variable and using the Converter's convertCurrency() function to pass those strings as key values to access the appropriate conversion rate and passing that Cents value to use the conversion rate on. How else can I pass the amount? \$\endgroup\$
    – Atom
    Aug 24, 2013 at 2:23

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.