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

Currency converter

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
convertercurrencystackoverflow

Problem

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 

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

        #endif


Converter.cpp

```
#include
#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["e

Solution

-
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

-
This form of casting:

conversion *= (double)amount;


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

conversion *= static_cast(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.

Code Snippets

std::string Converter::getSymbol(std::string const& currency) {}
long getLong() const { return outputCents; }
conversion *= (double)amount;
conversion *= static_cast<double>(amount);
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);

Context

StackExchange Code Review Q#30061, answer score: 7

Revisions (0)

No revisions yet.