Resolving lambdas that depend on each other

Question

In the C++20 codebase that I work there are a couple of functions which have helper functions defined as lambdas. I can see the idea of locality for these helpers. By having them there in the function, one doesn't have to care about them in other parts of the code.

As the code is proprietary, I cannot show the original; the following is a mock example. We have various vectors from the program and corresponding reference vectors. We want to compare actual and expected and then replace them with expected such that the program continues as planned after the checkpoint.

This is the code example:

#include <cmath>
#include <iostream>
#include <string>
#include <vector>

void checkpoint(std::vector<double> &actual,
                std::vector<double> const &expected, int const context_1,
                int const context_2, int const context_3) {
  auto report_deviation = [&](std::string const &type) {
    std::cout << "Deviation in " << context_1 << " " << context_2 << " "
              << context_3 << " of type " << type << "." << std::endl;
  };

  auto compare_and_replace = [&](double &actual, double const expected,
                                 double const tolerance,
                                 std::string const &type) {
    if (std::abs(actual - expected) > tolerance) {
      report_deviation(type);
    }
    actual = expected;
  };

  auto compare_and_replace_multi =
      [&](std::vector<double> &actual, std::vector<double> const &expected,
          double const tolerance, std::string const &type) {
        for (int i = 0; i != actual.size(); ++i) {
          compare_and_replace(actual[i], expected[i], tolerance, type);
        }
      };

  compare_and_replace_multi(actual, expected, 0.1, "Widget");
}

My gut is unhappy with this code and I cannot put my finger on it. There are a couple of things that I could rationalize:

• The nested closures increase the length of the function. In the aim of providing more cohesion, the original author has created something that feels more complex to me.
• The lambdas capture with [&], which is the most general form possible. The amount of state captured isn't specified or limited. Looking more closely the first report_deviation() only really needs the context variables. The compare_and_replace() only needs report_deviation() and then compare_and_replace_multi() only needs compare_and_replace(). In code I see a function and three functor classes with coupling among them. And I learned to minimize coupling over all other things.
• Although there is so much closure capturing going on, the functions still take four arguments each. They pass it to the next function. In Clean Code Robert C. Martin states that three arguments are already a lot, and more than that is a clear sign for too much complexity. The code has many function with 20 parameters, so there is a pattern there.
• I cannot test the comparison logic with mock data. I can only call the checkpoint() which then not only needs to have the data but also some context. In the actual example the checkpoint() doesn't directly get the data but in some nested structure and unpacks it before calling compare_and_replace_multi(). This code therefore feels untestable. And at the moment there are no tests for it.

Clean Code, which I currently read, seems to be from a Java perspective. And that might account for a preference to put everything into classes. But I do see the point that shared function arguments could be state of a class. My colleague argues that a class would need to have some non-trivial invariants to justify making it a class. And here there apparently aren't any cool invariants like std::vector has.

Still I find that one could extract a comparator class here that would be testable and independent of the other stuff.

#include <cmath>
#include <iostream>
#include <string>
#include <vector>

struct Context {
  int context_1;
  int context_2;
  int context_3;
};

class Comparator {
public:
  Comparator(double const tolerance, std::string const &type,
             Context const &context)
      : tolerance(tolerance), type(type), context(context) {}

  void compare_and_replace_multi(std::vector<double> &actual,
                                 std::vector<double> const &expected) {
    for (int i = 0; i != actual.size(); ++i) {
      compare_and_replace(actual[i], expected[i], tolerance, type);
    }
  }

  void compare_and_replace(double &actual, double const expected) {
    if (std::abs(actual - expected) > tolerance) {
      report(type);
    }
    actual = expected;
  };

  void report() {
    std::cout << "Deviation in " << context_1 << " " << context_2 << " "
              << context_3 << " of type " << type << "." << std::endl;
  }

private:
  double tolerance;
  std::string type;
  Context context;
};

std::vector<double> get_actual();
std::vector<double> get_expected();

void checkpoint(std::vector<double> &actual,
                std::vector<double> const &expected, int const context_1,
                int const context_2, int const context_3) {
  Context context = {context_1, context_2, context_3};
  Comparator comparator(0.1, "Widget", context);
  auto actual = get_actual(context_1);
  auto expected = get_expected(context_2);
  comparator.compare_and_replace_multi(actual, expected);
}

This would be a translation of the code into a class such that not much of the calling code would have to be changed. But the reporting actually is a different concern, so it should rather be this:

#include <cmath>
#include <iostream>
#include <string>
#include <vector>

class Comparator {
public:
  Comparator(double const tolerance, std::string const &type)
      : tolerance(tolerance), type(type) {}

  bool compare_and_replace_multi(std::vector<double> &actual,
                                 std::vector<double> const &expected) {
    bool has_deviation = false;
    for (int i = 0; i != actual.size(); ++i) {
      has_deviation |=
          compare_and_replace(actual[i], expected[i], tolerance, type);
    }
    return has_deviation;
  }

  bool compare_and_replace(double &actual, double const expected) {
    bool has_deviation = false;
    if (std::abs(actual - expected) > tolerance) {
      report(type);
      has_deviation = true
    }
    actual = expected;
    return has_deviation;
  };

private:
  double tolerance;
  std::string type;
  ;
};

struct Context {
  int context_1;
  int context_2;
  int context_3;
};

void report(Context const &context) {
  std::cout << "Deviation in " << context_1 << " " << context_2 << " "
            << context_3 << " of type " << type << "." << std::endl;
}

std::vector<double> get_actual();
std::vector<double> get_expected();

void checkpoint(std::vector<double> &actual,
                std::vector<double> const &expected, int const context_1,
                int const context_2, int const context_3) {
  Comparator comparator(0.1, "Widget", context);
  auto actual = get_actual(context_1);
  auto expected = get_expected(context_2);
  Context context = {context_1, context_2, context_3};
  if (comparator.compare_and_replace_multi(actual, expected)) {
    report(context);
  }
}

Is this a clear improvement over the previous code? Or am I approaching all this from the wrong angle?

Answer 1

Despite capturing everything, you aren't capturing enough. By doing that, you can pull things apart.

#include <cmath>
#include <iostream>
#include <string>
#include <vector>

auto make_report_deviation(int context_1, int context_2, int context_3) {
  return [=](std::string const &type) {
    std::cout << "Deviation in " << context_1 << " " << context_2 << " " << context_3 << " of type " << type << "." << std::endl;
  };
}

using ReportDeviation = std::function<void(std::string const&)>;

auto make_compare_and_replace(ReportDeviation report_deviation, double tolerance, std::string type) {
  return [=](double actual, double expected) {
    if (std::abs(actual - expected) > tolerance) {
      report_deviation(type);
    }
    return expected;
  }
}

As an aside, your compare_and_replace_multi is fairly similar to the BinaryOperation overload of std::transform.

void checkpoint(std::vector<double> &actual, std::vector<double> const &expected, ReportDeviation report_deviation) {
  auto compare_and_replace = make_compare_and_replace(report_deviation, 0.1, "Widget");
  std::transform(actual.begin(), actual.end(), expected.begin(), actual.begin(), compare_and_replace);
}

I'd also want to have some checking that expected.size() was at least actual.size()