patterncppMinor
Currency converter
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
Converter.h
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
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);
};
#endifConverter.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
-
-
More importantly, it should actually be removed as it's bad for encapsulation (accessors, also mutators, expose the implementation details).
Generally-speaking, no one is ever obligated to use your own driver. Your program should work with any sensible, non-breaking driver.
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.