6
\$\begingroup\$

I have a function that compares two doubles to N decimal places.

#include <cmath>
#include <iostream>

using namespace std;

enum class RelType : int { Less, Equal, Greater };

RelType _compare(double v1, double v2, int multiplier)
{
    if (std::round(v1 * multiplier) == std::round(v2 * multiplier))
        return RelType::Equal;
    if (v1 < v2)
        return RelType::Less;
    return RelType::Greater;
}

template<int N> RelType compare(double, double) { static_assert(false, "Not implemented."); }
template<> RelType compare<5>(double v1, double v2) { return _compare(v1, v2, 100000); }

int main(int argc, char* argv[])
{
    double v1 = 1.0;                        // 1.0000000000000000
    double v2 = (1.0 / 13.67) * 13.67;      // 0.99999999999999989

    cout << std::boolalpha;
    cout << (v1 == v2) << endl;                               // false (not what we expected)
    cout << (compare<5>(v1, v2)==RelType::Equal) << endl;     // true (correct behaviour)
    cout << (v1 > v2) << endl;                                // true (not what we expected)
    cout << (compare<5>(v1, v2)==RelType::Greater) << endl;   // false (correct behaviour)

    return 0;
}

My main two questions are about the efficiency and correctness of the _compare helper function. Are there any pitfalls to consider that might not be obvious at first glance?

\$\endgroup\$
  • \$\begingroup\$ What's the purpose of the 5 in compare<5>()? I don't get it... \$\endgroup\$ – glampert Mar 2 '15 at 2:14
  • \$\begingroup\$ The template parameter denotes the number of decimal places to be compared. \$\endgroup\$ – Frank Mar 2 '15 at 2:18
  • \$\begingroup\$ The N parameter defines what the multiplier argument to _compare will be: multiplier = 10 to the power of N \$\endgroup\$ – Frank Mar 2 '15 at 2:29
  • \$\begingroup\$ Okay, I see that now, but the value 100000 is hardcoded, so the template parameter serves not actual purpose. You could very well remove those templates and rename a single function to compare_5_decimal_places() or similar. The template functions are certainly unclear. \$\endgroup\$ – glampert Mar 2 '15 at 2:32
  • \$\begingroup\$ It is important to note that this compare function specifically compares rounded values. Please see Ian's answer below. \$\endgroup\$ – Frank Mar 3 '15 at 19:25
7
\$\begingroup\$

The main problem I have with your code is with these templates:

template<int N> RelType compare(double, double) { static_assert(false, "Not implemented."); }
template<> RelType compare<5>(double v1, double v2) { return _compare(v1, v2, 100000); }

They are misleading. The 5 in the second function should be supposed to denote the number of decimal places to be compared, however, it is not implemented to do so, being only used as name decoration. E.g.:

// Comparing 5 decimal places:
cout << (compare<5>(v1, v2) == RelType::Equal) << endl;

// Also comparing 5 decimal places:
// (In actuality, a shady compile error due 
// to the static_assert. But we can do better...)
cout << (compare<8>(v1, v2) == RelType::Equal) << endl;

I see no point in having a parameter if it always has to have the same value, so one improvement to the interface would be just getting rid of the templates and having a single function that always compares with 5 decimal places and no other option.

Another possibility, if you need to have the ability to configure the number of decimal palaces at compile time, would be actually using the N parameter to compute the multiplier, instead of hardcoding a value. You can do so with a compile-time power function:

template<int N> RelType compare(double v1, double v1)
{
    constexpr auto multiplier = compile_time_pow(10, N);
    // ... the rest ...
} 

Note that in the above link C++11 is utilized, though it is probably possible to achieve the same without constexpr.

\$\endgroup\$
  • \$\begingroup\$ Thank you. compile_time_pow is what I am looking for. Awesome! \$\endgroup\$ – Frank Mar 2 '15 at 3:08
5
\$\begingroup\$

You can use a generalized function suggested by @glampert

constexpr auto multiplier = compile_time_pow(10, N);

You can also use a simplified version that assumes the number 10.

template <int N> struct Multiplier
{
   static const int value = 10 * Multiplier<N-1>::value;
};

template <> struct Multiplier<0>
{
   static const int value = 1;
};

template <int N> RelType compare(double v1, double v2)
{
   return _compare(v1, v2, Multiplier<N>::value);
}
\$\endgroup\$
5
\$\begingroup\$

With regards to the correctness of _compare(), it depends what precisely you want to achieve.

In your current implementation:

compare<0>(2.499999, 2.500001)

returns RelType::Less

If this is the answer you want - then fine. However it seems a little odd to say that two numbers which differ by 2e-6, are not equal at 0 decimal places precision.

Instead try

RelType _compare(double v1, double v2, int multiplier)
{
    double margin = vcl_pow(0.1, multiplier);
    if (v1+margin < v2)
        return RelType::Less;
    if (v2+margin < v1
        return RelType::Greater;
    return RelType::Equal;
}

Of course, all this can be improved with a little constexpr magic, but glampert's answer has that covered.

One other point to note regarding correctness: C++ bans identifiers in the global namespace beginning with an underscore, giving your current code undefined behaviour according to the standard.

\$\endgroup\$
  • \$\begingroup\$ Interesting points, thank you. There are different use cases for comparing doubles. Often you just want to see if their differences fall within a small margin, as you suggested. Imagine I'm printing 2.499999 and 2.500001 to the console to zero decimal places. This will output 2 and 3, and I would like compare<0> to give a result consistent with the printed output. For clarity, I will rename the function to compareRounded. Agree with the namespaces comment. \$\endgroup\$ – Frank Mar 2 '15 at 21:50
3
\$\begingroup\$

The first problem you have is using namespace std;. It is not a good idea to using namespaces in C++ as it can cause problems when multiple functions have the same name. More can be read about this at Stack Overflow.

It is a good idea to use braces around single-line if and loop statements to help ensure the statements go in the correct place:

if (std::round(v1 * multiplier) == std::round(v2 * multiplier))
    return RelType::Equal;
if (v1 < v2)
    return RelType::Less;
return RelType::Greater;

This appears to be a source of potential errors:

template<> RelType compare<5>(double v1, double v2) { return _compare(v1, v2, 100000); }

Instead of passing the value needed (100000), you should probably pass the same value as specified in the <> section - 5. Then, you need to change _compare to calculate the multiplier value with the pow function in cmath. This will reduce the number of errors introduced by the programmer mentally calculating or mistyping the value of 10^N needed.

\$\endgroup\$
  • \$\begingroup\$ I want the answer to 10^N at compile time. A requirement is for this function to be efficient and calculating 10^N every time the function is called is needlessly inefficient. \$\endgroup\$ – Frank Mar 2 '15 at 2:35
  • \$\begingroup\$ OK, I see. However, manually passing the value is an obvious place for errors to occur. \$\endgroup\$ – Hosch250 Mar 2 '15 at 2:37
  • \$\begingroup\$ What would be nice is to have a metafunction that can calculate the multiplier from N. \$\endgroup\$ – Frank Mar 2 '15 at 2:47
  • \$\begingroup\$ I'm not that advanced at C++, so I'm not really sure what you are talking about. However, it sounds like a good idea. \$\endgroup\$ – Hosch250 Mar 2 '15 at 2:49
  • \$\begingroup\$ @FrankHippmann See glampert's answer, he has just what you need. \$\endgroup\$ – Hosch250 Mar 2 '15 at 2:55

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.