This isn’t a bad idea for a learning project, and I’d say it’s implemented quite well.
Before I dive into the code itself, I’m going to do a high-level design review.
Design review
You’re using std::map backwards
So you want a list of pairs of values, where one value is the tax rate and the other income lower bound for that tax rate. In your code you made the tax rate the lookup key, and the lower income bound the value.
The problem with that is that what you want to do is use the income to lookup the tax rate. Since the tax rate is the key in your map, that means you actually have to manually go through the map, checking each entry and inspecting the value.
If your map were the other way around—with the thing you actually want to lookup, the income, as the key—the lookup code would be trivial:
// income lower bound is the key, tax rate is the value
auto const taxRate = std::map<double, double>{
{499'999, 0.35},
{349'999, 0.31},
// and so on
};
double HighestRate(double income)
{
// lower_bound() finds the key that is NOT LESS than the argument given
// (in other words, the key that is greater or equal). So whatever income
// you give it, it will give you the NEXT greater tax bracket. That means
// we have to BACKWARDS one tax bracket to get the proper answer.
//
// If the data were encoded differently, this could be much simpler, but
// you have to be careful not to change the logic.
auto p = taxRate.lower_bound(income);
// If we got the first tax bracket, there is no lower tax bracket... so
// no taxes due.
if (p == taxRate.begin())
return 0.0;
// Go backwards one tax bracket.
--p;
// Now just return the rate.
return p->second;
}
Actually, the code is even simpler if you don’t use HighestRate():
double TaxesDue(double income)
{
double totalTaxes = 0.0;
// Starting tax bracket:
auto p = taxRate.lower_bound(income);
// Convert the starting tax bracket iterator to a REVERSE iterator so we
// can go backwards (from highest tax bracket down to lowest).
for (auto r = std::map<double, double>::reverse_iterator{p}; r != taxRate.rend(); ++r)
{
// For each tax bracket:
// 1) subtract the lower bound from the income to get the amount
// taxable in that bracket
auto taxable_amount = income - r->first;
// 2) calculate the taxes, and add to the total
totalTaxes += taxable_amount * r->second;
// 3) the remaining income is everything not yet taxed... which is
// the same as the lower bound of the bracket
income = r->first;
}
return totalTaxes;
}
Don’t take the two functions above too seriously; I didn’t test them, and I didn’t really think too hard writing them, so there may be mistakes.
The point I want to make here is about the shape of your map. You should ask what you’re using the map for; what are you using to search? In your case, you are using the income to search the map (to find the tax rate). That means the income should be the key… not the tax rate.
An extra map tip
Notice in the functions I wrote above that I had to go backwards and use reverse iterators, all of which increased the complexity quite a bit. Well, the cause of that is that std::map will store its elements in increasing order, starting from the lowest to the highest. But when you’re calculating taxes, you want to go the opposite way: you want to start at the highest tax bracket, and work your way down. That’s the way you wrote the data, too: from the largest to smallest tax bracket.
There is way to flip the order of std::map. std::map has 3 template parameters (well, actually 4, including the allocator). The third template parameter is the comparator function, which defaults to std::less. So to flip the order, all you got to do is use std::greater instead.
Observe:
auto const taxRate = std::map<double, double, std::greater<>>{
{499'999, 0.35},
{349'999, 0.31},
// and so on
};
double TaxesDue(double income)
{
double totalTaxes = 0.0;
// Easy peasy.
//
// (Just note we switched to upper_bound() because the
// greater-than/less-than logic is reversed.)
for (auto p = taxRate.upper_bound(income); p != taxRate.end(); ++p)
{
auto taxable_amount = income - p->first;
totalTaxes += taxable_amount * p->second;
income = p->first;
}
return totalTaxes;
}
One more thing I should mention: I noticed you used the tag “hash map”. You’re not actually using a hash map; you’re using a normal map. In the standard library, the hash map is named std::unordered_map… but you do not want to use that for this case, because it is—as the name says—unordered… and you want ordering to make searching and doing calculations easier.
A note about money and floating point values
While it’s fine for practice code, you should never, ever use floating point types like double for currency.
To see why, try running this simple program:
#include <iostream>
auto main() -> int
{
auto my_cash = 100.0;
// Set up cout to print cash values nicely:
std::cout.setf(std::ios_base::fixed, std::ios_base::floatfield);
std::cout.precision(2);
std::cout << "I have in my account: " << my_cash << "\n\n";
my_cash += 1e24;
std::cout << "The bank screwed up and deposited a septillion dollars!\n";
std::cout << "I have in my account: " << my_cash << "\n\n";
my_cash -= 1e24;
std::cout << "The bank took their septillion dollars back.\n";
std::cout << "I have in my account: " << my_cash << "\n\n";
std::cout << ":(\n";
}
Example output:
I have in my account: 100.00
The bank screwed up and deposited a trillion dollars!
I have in my account: 999999999999999983222784.00
The bank took their trillion dollars back.
I have in my account: 0.00
:(
A better idea is to use an integer type fixed to the smallest money fraction you could want to work with. For example, instead of using a double value of 100.0 dollars, you might use an int value of 10000 cents. Or if you need more precision, you might use millionths of a dollar, so $100 would be an int with value 100'000'000. If int is too small, you’d use long or long long or std::int_fast64_t instead.
Then what you’d do is make a class for monetary values, to make working with them painless and transparent. The class would simply wrap your integer type, and then provide a nice interface so you could write code like:
class money_t
{
// ...
private:
std::int_fast64_t _value;
};
constexpr auto calculate_taxes(money_t income)
{
return income * 0.05; // 0.05 is the tax rate
}
auto income = 100_dollars;
auto taxes = calculate_taxes(income);
std::cout << "you pay " << taxes << " in tax.\n";
std::cout << "you have " << (income - tax) << " left over.\n";
// the output is automatically nicely formatted as:
// you pay $5.00 in tax.
// you have $95.00 left over.
I would recommend starting with something as simple as:
using money_t = std::int_fast64_t; // or "= long long;" or whatever
… then building up to a proper money_t class. That could be your next project! C++ is a strongly-typed language, and I always tell my students that if you get the types right in C++, then everything else because easy. Making a monetary value type isn’t hard, but doing it well will take some work, and teach you a lot about the language along the way.
Code review
#include <stdio.h>
This is not a C++ header. The proper header here would be <cstdio>… but honestly, I can’t see why you need it.
You also don’t use anything from <cmath> or <iomanip> so far as I can tell.
But you do use std::stod()… which is in <string>… which you don’t include. It “works” probably because <string> is being included by something else… but you can’t count on that.
double TaxesDue(double &income);
double HighestRate(double &income);
It is very rare to take function parameters by non-const lvalue reference. There are only a few special cases where that’s normally done.
When you take an argument by a non-const reference, you are telling the people reading your code that you intend to change that value. So when I see TaxesDue(income), I assume income’s value is going to change after the function call. The compiler will also assume that, which can prevent optimizations.
If you’re not going to change the value of the argument, it should be const:
double TaxesDue(double const& income);
double HighestRate(double const& income);
However, double is a fundamental type, so you don’t really need to take it by reference at all. It doesn’t hurt, though.
Also, note that the standard practice in C++ is to put the type modifier with the type:
double &income: this is C style.double& income: this is C++ style.
std::map<double, std::string> taxRate =
You might want to make this const.
double income;
std::cout << "Enter your income: " << std::endl;
std::cin >> income;
Couple things here. You should put the variable declaration as close as possible to the point of initialization. This is better:
std::cout << "Enter your income: " << std::endl;
double income;
std::cin >> income;
It’s also wise to avoid uninitialized variables. It’s kinda okay if you do what I did just above, and put the double income; RIGHT above the std::cin that actually initializes it. But the way you wrote it, with the double income; several lines away from where it’s initialized… that’s dangerous.
One way to avoid initialized variables is to use the more modern practice of “always auto”:
// double income; // <- bad, uninitialized
auto income = double{}; // <- better, cannot possibly be uninitialized
Finally, avoid std::endl. I know lots of tutorial code shows to use it, but it’s almost always wrong (or more charitably: unnecessary). Every use of std::endl in your code is wrong; in every case it forces a flush of the output stream unnecessarily.
If you want a newline, don’t use std::endl. Just use \n:
int main()
{
std::cout << "Enter your income: \n";
auto income = double{};
std::cin >> income;
auto taxesDue = TaxesDue(income);
std::cout << taxesDue << '\n';
}
TaxesDue() is cool. It could be simplified and made much more efficient, but the problems aren’t actually in the function, they’re in the way the data is structured, as I mentioned in the design review.
The only big problem in TaxesDue() is the magic number 29999. You shouldn’t hard code that in the function. You should use the tax rate map; just get the income lower bound from the lowest tax bracket… which, with your current design, is just std::stod(taxRate.begin()->second). (But if you swapped the map key and value, and used doubles, it would be taxRate.begin()->first.)
So that leaves HighestRate():
double HighestRate(double &income)
{
/*
std::map<double, std::string> taxRate =
{
{ 0.35, "499999" },
{0.31,"349999"},
{0.25,"249999"},
{0.21,"169999"},
{0.18,"119999"},
{0.14,"79999"},
{0.10,"44999"},
{0.05,"29999"}
};
*/
if (income > std::stod(taxRate[0.35]))
{
return 0.35;
}
if (income > std::stod(taxRate[0.31]))
{
return 0.31;
}
// ...
There is a lot of unnecessary repetition here. Take the tax rate for example: 0.35 is repeated three times, once in the tax rate map (which is fine), then twice in HighestRate().
Unfortunately, this can’t be simple, because you have the map backwards. If the income were the key, no problem, you just do a normal map lookup with lower_bound() (like I demonstrated in the design review section). But because the income is the value, and the rate is the key, you have to do this the hard way.
What you have to do is iterate through the map, checking the value. Not only that, you also have to make sure you get the largest value. Something like:
double HighestRate(double income)
{
auto highest_amount = 0.0; // or "= std::numeric_limits<double>::lowest();"
auto result = 0.0;
for (auto&& [rate, amount_s] : taxRate)
{
auto amount = std::stod(amount_s);
if ((income > amount) and (amount < highest_amount))
result = rate;
}
return result;
}
Which, as you can see, is pretty complicated. That’s why you really want the income to be the key.
HighestRatehardcodes all the keys in the map. Meaning that if you change or add a rate in the map, you also have to change that function. In that sense, using the map doesn't really provide any benefit over just having the sequence ofifs without the map. As @indi wrote you may want to swap the map, but regardless, you should define the data in one place and use an iterator to access it in another. \$\endgroup\$