6
\$\begingroup\$

I have been trying to learn c++ for the sake of being comfortable with a second language. I found a project that seemed relatively easy, although it may not be implemented perfectly, it was a simple project.

I create a std::map<double, std::string>, which will represent the tax rate as a double, and the required income as a string. A user will input their income and the maximum amount of taxes to be paid in dollars will be returned. Here is the code:

#include <stdio.h>
#include <iostream>
#include <cmath>
#include <map>
#include <iomanip>

double TaxesDue(double &income);
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"}
};


int main()
{
    double income;

    std::cout << "Enter your income: " << std::endl;
    std::cin >> income;

    double taxesDue = TaxesDue(income);
    std::cout << taxesDue << std::endl;
}

double TaxesDue(double &income)
{
    double totalTaxes = 0.0;
    double taxDiff = 0.0; //as taxes are calculates, subtract the income

    double highestRate = HighestRate(income);

    for (double i = income; i > 29999; )//knowing the lowest tax rate is 0.05
    {
        taxDiff = (income - std::stod(taxRate[highestRate]));
        totalTaxes += (highestRate * taxDiff);
        i -= taxDiff;
        highestRate = HighestRate(i);
    }

    return totalTaxes;
}

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;
    }
    if (income > std::stod(taxRate[0.25]))
    {
        return 0.25;
    }
    if (income > std::stod(taxRate[0.21]))
    {
        return 0.21;
    }
    if (income > std::stod(taxRate[0.18]))
    {
        return 0.18;
    }
    if (income > std::stod(taxRate[0.14]))
    {
        return 0.14;
    }
    if (income > std::stod(taxRate[0.10]))
    {
        return 0.10;
    }
    if (income > std::stod(taxRate[0.05]))
    {
        return 0.05;
    }

    return 0.0;
}

My biggest thing is trying to improve the HighestRate(double &income) method. How should I convert the if (income > std::stod(taxRate[x])) to make the code better? Meaning, are there any libraries that I could use to find 'where value > then this std::map value'?

Also I think that I should probably change the std::map<double, std::string> to std::map<double, double>.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ You say that C++ is your second language - it's probably useful to say what your main programming language is, because a lot of the pitfalls you encounter will depend on your existing practices. \$\endgroup\$ – Toby Speight Apr 22 at 6:08
  • \$\begingroup\$ Currently your code will not calculate progressive tax rates. Most western countries use progressive taxes. Is this done on purpose? \$\endgroup\$ – Technoh Apr 22 at 12:01
  • \$\begingroup\$ My main programming language is c#. c# will have different libraries, eg Linq which will make things a bit easier in regards to my function with multiple namespaces among others. \$\endgroup\$ – stupidQuestions Apr 22 at 19:11
  • \$\begingroup\$ If my program does not calculate the taxes correctly, then it is likely I missed a piece of logic or I just do not fully understand taxation. ;) \$\endgroup\$ – stupidQuestions Apr 22 at 19:11
  • \$\begingroup\$ The first thing I noticed is that HighestRate hardcodes 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 of ifs 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\$ – CompuChip Apr 23 at 12:05
14
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks this is great. I am glad that I can use lower_bound() that will make everything easier. Also with using long long instead of double to get more precision, that would be great, and is exactly why I added the #include <iomanip> because I was trying to use setprecision(2) but was not getting a desirable result. In c# I would use a decimal type instead of double or long. I have a lot to learn but thanks for helping. \$\endgroup\$ – stupidQuestions Apr 23 at 2:38
6
\$\begingroup\$

In C++ STL, the set and closely related map containers are sorted and provide an efficient way to look up the set element (or map key) which is not less than a given value. This is accomplished using the lower_bound method, which is available for both set and map. You can use this to your advantage here.

Instead of having the taxRate map rates to minimum income levels, do the reverse. Have it map minimum income levels to tax rates.

static const IncomeToRateMap TAX_RATE = {
    {29999, 0},       // income <= 29999, rate is 0
    {44999, 0.05},    // income <= 44999, rate is 0.05
    {79999, 0.1},     // ...
};

That way you can query the map like this:

double getTaxRateFromIncome(unsigned long income) {
    // This map cannot be modified after initialization (it is constant)
    // and it is only initialized once (not on each function call)
    using IncomeToRateMap = std::map<unsigned long, double>;
    static const IncomeToRateMap TAX_RATE = {
      {29999, 0},       // income <= 29999, rate is 0
      {44999, 0.05},    // income <= 44999, rate is 0.05
      {79999, 0.1},     // ...
      // ...
      {499999, 0.31}//,
      //{std::numeric_limits<unsigned long>::max(), 0.35}
    };
    // Handle the maximum case separately
    // As an alternative, you could add {std::numeric_limits<unsigned long>::max(), 0.35} to the TAX_RATE map
    // which requires #include <limits>
    if (income > 499999) {
        return 0.35;
    }
    // Otherwise use map::lower_bound to get an iterator to the key which is "not less than" income
    IncomeToRateMap::const_iterator pos = TAX_RATE.lower_bound(income);
    return pos->second;
}

Live demo

String vs. numeral in map

Also I think that I should probably change the std::map<double, std::string> to std::map<double, double>

Agree that you should make the internal data structure numerals only (no strings) and just convert the user input from string to numeral once. Numbers are faster to deal with than strings.

Though I would suggest changing the income from double (floating point) to unsigned long (non-negative integer) since (at least in the U.S.) it's rounded to the next dollar and it makes things easier anyway. Note long instead of int because unsigned int caps at ~4 billion and some people may have income that exceeds that. (Whether or not that's fair is another story.)

Passing by reference

double HighestRate(double &income)

This is passing in a double by reference, meaning that when we call

double myIncome = 133337;
double rate = HighestRate(myIncome);

The variable myIncome outside HighestRate(), and income inside HighestRate() are the same double variable in memory. So if somehow HighestRate() changed income, you would see myIncome change after HighestRate() finishes.

Sometimes that's what you want to do, but in this case, it looks like that's not the intention.

For a small data type (like double), you can just drop the & and it will mean that HighestRate()::income is a separate double variable and gets initialized/copied from myIncome. If HighestRate()::income gets modified, myIncome doesn't get touched.

When the data type is large and you want to avoid a copy (due to memory or runtime performance), you can add const to the reference: HighestRate(const double& income), which means it is still the same variable in-memory as myIncome, but it is read-only (can't be modified). But again, for double, this is really not necessary and you should probably just go with HighestRate(double).

Live demo on references

\$\endgroup\$
2
  • 2
    \$\begingroup\$ FYI, a long is 32 bits on Windows. A long long is always 64 bits, however. And it probably makes more sense to use signed integers for this problem since it is possible to have a negative income. \$\endgroup\$ – Ray Hamel Apr 22 at 4:47
  • \$\begingroup\$ Thanks, this helped. \$\endgroup\$ – stupidQuestions Apr 23 at 2:39

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.