Skip to main content
Minor re-wording
Source Link
Toby Speight
  • 88.4k
  • 14
  • 104
  • 327

Although the using_*() members are marked const, users might forget that and assume that they modify an instance rather than returning a new one (that'swhich wouldn't actually notbe possible, because the new object has a different type, in general).

Although the using_*() members are marked const, users might forget that and assume that they modify an instance rather than returning a new one (that's actually not possible, because the new object has a different type, in general).

Although the using_*() members are marked const, users might forget that and assume that they modify an instance rather than returning a new one (which wouldn't actually be possible, because the new object has a different type, in general).

Source Link
Toby Speight
  • 88.4k
  • 14
  • 104
  • 327

Even though I left the code overnight before posting (that always works; I wake up with improvements in my head) I still spotted a couple of improvements.

Avoid misunderstanding of the builder interface

Although the using_*() members are marked const, users might forget that and assume that they modify an instance rather than returning a new one (that's actually not possible, because the new object has a different type, in general).

That leads to them writing code like this:

auto m = stats::median;
m.using_inplace_strategy();  // WRONG
m(values);

We can make that an error by using a [[nodiscard]] attribute on those functions.

Simplify use of SCOPED_TRACE()

Instead of having to create small blocks like this:

{
    SCOPED_TRACE("default strategy");
    m(values);
}
{
    SCOPED_TRACE("copy strategy");
    m.using_copy_strategy()(values);
}
{
    SCOPED_TRACE("external strategy");
    m.using_external_strategy()(values);
}

A small helper function can reduce the repetition:

static void test_strategy(auto const& m, auto&& range, auto&& name = "")
{
    SCOPED_TRACE(name);
    m(std::forward<decltype(range)>(range));
}

And we use in in five places thus:

test_strategy(m, values, "default strategy");
test_strategy(m.using_copy_strategy(), values, "copy strategy");
test_strategy(m.using_external_strategy(), values, "external strategy");

Test custom compare and projection using all strategies

We only tested these using the default strategy. Instead, we should use test_values to test all strategies:

TEST(Median, CustomOrder)
{
    auto values = std::array{3, 4, 5, 100, 101, 102};
    // order by last digit:  100, 101, 102, 3, 4, 5
    auto const compare = [](int a, int b){ return a % 10 < b % 10; };
    SCOPED_TRACE("from here\n");
    test_values(values, {102, 3}, compare);
}

TEST(Median, CustomProjection)
{
    auto values = std::array{3, 4, 5, 100, 101, 102};
    // project to last digit:  0, 1, 2, 3, 4, 5
    auto const projection = [](int a){ return a % 10; };
    SCOPED_TRACE("from here\n");
    test_values(values, {2, 3}, {}, projection);
}

Ensure strategy is not used when NaN values are present

Just add a call to test_values_trivial:

TEST(Median, NaNsFirst)
{
    constexpr auto nan = std::numeric_limits<double>::quiet_NaN();
    double values[] = { nan, nan, 1, 1, 100, 100, 10 };
    test_values_trivial(values, test::dummy_midpoint{});
    EXPECT_TRUE(std::isnan(stats::median(values)));
}

Duplicate test

TEST(Median, ProjectByValue) tests exactly the same functionality as TEST(Median, CustomProjection) so can be dropped. Transparent projection is the default, so that's also thoroughly tested.