3
\$\begingroup\$

I'm trying to make a simple header only C++ library for interval arithmetic as a programming exercise.

There are some subtle aspects of interval arithmetic (like floating point rounding mode) that I am deliberately ignoring for now.

Eventually I want to templatize this thing, but that forces me to face a design question, "what do I do for integer overflow?", that I don't have a good answer to.

Here is macros.hpp, it has some really trivial macros that I like. They make things like logical and arithmetic negation stand out more visually. Is this bad form? MAX and MIN are not used here, but will be more useful when I implement multiplication.

#ifndef MACROS_HPP
#define MACROS_HPP

#define NOT(x) \
    (!(x))

#define MAX(x,y) \
    ((x > y) ? (x) : (y))

#define MIN(x,y) \
    ((x < y) ? (x) : (y))

#define NEGATE(x) \
    (-(x))


#endif // MACROS_HPP

And here is interval.hpp

Intervals are mutable, but require that the upper bound is always greater than or equal to the lower bound.

There's probably a better way to handle errors than just throwing a runtime error when the Interval would be put into an invalid state. And I think using a method called init to set the upper and lower bound together rather than independently is counterintuitive. I'm just trying to make sure that a function or method elsewhere in the code that takes an interval as an argument does not need to check the bounds.

I'm a little confused about operator overloading. I'm not convinced that the way I implemented operator+ is idiomatic. I tried several times to make it return a reference so that I'm not returning a complex object by value, but am not sure how to make it work. I'm also not sure who is supposed to own the memory in that case.

#ifndef INTERVAL_HPP
#define INTERVAL_HPP
#include <stdexcept>
#include "macros.hpp"


// Represents an interval of Real numbers,
// Or anything that's orderable and supports numeric
// operations.
//
// The default interval is [0,0]
//
// The operations lower, upper, and init can be used
// to change the bounds of a given interval, but the
// interval invariant (lower <= upper) is always enforced.

class Interval {
    public:
        // construct zero interval [0,0]
        explicit Interval();
        // get lower bound
        double lower() const;
        // set lower bound, fails if larger than upper bound
        void lower(double);
        // get upper bound
        double upper() const;
        // set upper bound, fails if smaller than lower bound
        void upper(double);
        // set both bounds simultaneously, fail if [lower, upper]
        // is not a valid interval
        void init(double, double);
        // add two intervals together, yielding a new interval
        Interval operator+(const Interval&) const;
        // negate interval in place
        void negate();
    private:
        double m_lower;
        double m_upper;
};

Interval::Interval()
{
    this->m_lower = 0;
    this->m_upper = 0;
}

double Interval::lower() const
{
    return this->m_lower;
}

void Interval::lower(double l)
{
    if (l <= this->m_upper) {
        this->m_lower = l;
    } else {
        throw std::runtime_error("bad lower bound");
    }
}

double Interval::upper() const
{
    return this->m_upper;
}

void Interval::upper(double u)
{
    if (this->m_lower <= u) {
        this->m_upper = u;
    } else {
        throw std::runtime_error("bad upper bound");
    }
}

void Interval::init(double l, double u)
{
    if (l <= u) {
        this->m_lower = l;
        this->m_upper = u;
    } else {
        throw std::runtime_error("invalid range");
    }
}

Interval Interval::operator+(const Interval &b) const
{
    Interval out = Interval(); // zero
    out.init(
            this->lower() + b.lower(),
            this->upper() + b.upper()
    );
    return out;
}

void Interval::negate()
{
    this->init(
        NEGATE(this->upper()),
        NEGATE(this->lower())
    );
}

#endif // INTERVAL_HPP

At the risk of making this example far too long, this is the extremely rudimentary test suite I used to make sure that this snippet works. The test suite itself shouldn't be considered part of the "code under review", it's just here to support my claim that the library does roughly what I intend it to do.

Here is mytest.cpp

#include "interval.hpp"
#include <gtest/gtest.h>

TEST(INTERVAL_TEST, IT_ZERO) {
    double zero = 0.0;
    Interval it = Interval();
    EXPECT_EQ(it.lower(), zero);
    EXPECT_EQ(it.upper(), zero);
}


TEST(INTERVAL_TEST, IT_SETBOUNDS) {
    Interval it = Interval();
    it.upper(5.0);
    it.lower(5.0);
    EXPECT_EQ(it.lower(), 5.0);
    EXPECT_EQ(it.upper(), 5.0);
}


TEST(INTERVAL_TEST, IT_ADD_ZEROES) {
    double zero = 0.0;
    Interval zero1 = Interval();
    Interval zero2 = Interval();
    Interval result = zero1 + zero2;
    EXPECT_EQ(result.lower(), zero);
    EXPECT_EQ(result.upper(), zero);
}


TEST(INTERVAL_TEST, IT_ADD_NON_ZERO) {
    Interval a = Interval();
    a.init(1.0, 4.0);
    Interval b = Interval();
    b.init(2.0, 4.0);
    Interval result = a + b;
    EXPECT_EQ(result.lower(), 3.0);
    EXPECT_EQ(result.upper(), 8.0);
}



int main(int argc, char** argv)
{
    ::testing::InitGoogleTest(&argc, argv);
    int out;
    out = RUN_ALL_TESTS();
    return out;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ On your macros- there's subtle things that are wrong with them. For example if you do min(x++, y), x could get incremented twice if x is the lower value. This is one of the reasons not to use macros for this stuff. \$\endgroup\$ – Gabe Sechan Mar 28 '17 at 6:41
1
\$\begingroup\$

Here is macros.hpp, it has some really trivial macros that I like. They make things like logical and arithmetic negation stand out more visually. Is this bad form?

Yes this is bad form. A ! operator or a - operator already stand out enough; if they are not, then your code is too complicated.

This is easy to miss:

some_condition() && myValue == kMyExpected && myInteger == -10 && some_function() == 30 && !myBool

This is hard to miss:

!myBool

If you keep your code simple, it will be hard to miss arithmetic or logical negation.

Additionally, your MAX and MIN macros are rendered obsolete by std::max and std::min declared in the <algorithm> header. Rather than using MAX(a, b), it's std::max(a, b), and std::max is more robust than your MAX or MIN macros because they are macros.

TLDR: Don't use macros.


// Represents an interval of Real numbers,
// ...
// interval invariant (lower <= upper) is always enforced.

class Interval {

Nice comment! It's almost a documentation comment, though, so you might as well make it one (I'm assuming Doxygen format; if this is a doc-comment for some other documentation tool, disregard this):

/**
 * \brief Represents an interval of Real numbers.
 * \details Or anything that's orderable and supports numeric
 * operations.
 *
 * The default interval is [0,0]
 *
 * The operations lower, upper, and init can be used
 * to change the bounds of a given interval, but the
 * interval invariant (lower <= upper) is always enforced.
 */
class Interval {

(Or something like that)


// construct zero interval [0,0]
explicit Interval();
// get lower bound
double lower() const;
// set lower bound, fails if larger than upper bound
void lower(double);
// get upper bound
double upper() const;
// set upper bound, fails if smaller than lower bound
void upper(double);
// set both bounds simultaneously, fail if [lower, upper]
// is not a valid interval
void init(double, double);
// add two intervals together, yielding a new interval
Interval operator+(const Interval&) const;
// negate interval in place
void negate();

Considering that your class only contains two doubles, I'd instead make it immutable:

explicit Interval()
    : Interval(0, 0)
{}

explicit Interval(double lower, double upper)
    : m_lower{ lower }
    , m_upper{ upper }
{}

double lower() const { return m_lower; }
double upper() const { return m_upper; } // Yes all these functions can be implemented not inline.

Interval operator+(const Interval &rhs) const {
    return Interval{ m_lower + rhs.m_lower, m_upper + rhs.m_upper };
}

Interval negate() const {
    return Interval{ /* implementation */ };
}

On the other hand, if you do want the getter-setter mutable style, I personally find it easier to read if setters are set_*. But this comes down to taste.

void upper() const;
void set_upper(double newUpper);

Along those lines, you don't need a comment if the function is named well. For example, these two functions' comments:

// get upper bound
double upper() const;

// ...

// add two intervals together, yielding a new interval
Interval operator+(const Interval&) const;

They are unnecessary. It is clear from the function name what they do.


};

Interval::Interval()
{
    this->m_lower = 0;
    this->m_upper = 0;
}

Don't implement all the functions in the header file. Instead, create interval.cpp and implement the functions in there.


Good job writing unit tests! A couple things to note right off the bat:

#include <gtest/gtest.h>

I'm fairly sure that googletest recommends you use quotes on the include. Whichever way you choose is fine, though.

TEST(INTERVAL_TEST, IT_ZERO) {
    double zero = 0.0;
    Interval it = Interval();
    EXPECT_EQ(it.lower(), zero);
    EXPECT_EQ(it.upper(), zero);
}

This should be

TEST(IntervalTest, ConstructsToZero) {
    Interval it = Interval();
    EXPECT_EQ(it.lower(), 0.0);
    EXPECT_EQ(it.upper(), 0.0);
}

Extracting out the 0.0 into a variable is fruitless. Also, the naming convention googletest uses is PascalCase for test names (IIRC). Finally, you should use a more descriptive name for the test.

Finally, there is one real problem with your unit tests: You never test negative numbers.

When you test your code, keep in mind equivalence partitioning. Basically, just split the input into ranges and ensure you test something from each range as well as edge cases.

In your specific example, the Interval class consists of two doubles. A double is approximately a real number. The reals go from (-Inf, Inf), so the natural ranges to split into are the negative and positive numbers.

So I can partition the interval into:

[negative, negative]
[negative, positive]
[positive, negative] // I know this will fail, so I should add an EXPECT_THROWS
[positive, positive]

But this also disregards 0 - which could be called an edge case, so you need to test zero as well. Not too bad.

If you tested these cases, you'd cover most possible bugs. There are a couple more edge cases since we are working with floating point numbers, but from someone learning to code, I wouldn't be upset if they didn't cover them.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback. I just now added a test to make sure that negation works and I can't trigger the failure you described. This test, for instance, passes. TEST(interval_test, it_negate_asym) { Interval a = Interval(); a.init(-1.0, +2.0); a.negate(); EXPECT_EQ(a.lower(), -2.0); EXPECT_EQ(a.upper(), +1.0); } \$\endgroup\$ – Gregory Nisbet Mar 28 '17 at 6:40
  • 2
    \$\begingroup\$ @justin I disagree a bit about the getter/setter style. The way the OP did it is exactly how it's done with the c++ standard library classes. \$\endgroup\$ – πάντα ῥεῖ Mar 28 '17 at 6:45
  • \$\begingroup\$ @GregoryNisbet sorry I was mistaken. I misread your code and your code was correct \$\endgroup\$ – Justin Mar 28 '17 at 6:47
  • \$\begingroup\$ @Justin The reason I didn't think to add more tests (or, really, any) for negation is that - reverses all comparisons between numbers and I'm handling that by reversing upper and lower in the arguments to init. That is not at all obvious. How should I make negate more readable? \$\endgroup\$ – Gregory Nisbet Mar 28 '17 at 6:52
  • \$\begingroup\$ @πάνταῥεῖ That's true. It's not seen too often, but yeah stringstream does it with str() and vector (kinda) with at(). It's late at night, so I tend to make more mistakes. \$\endgroup\$ – Justin Mar 28 '17 at 6:54

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.