2

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?

2
  • Is the size of the mock example representative of the size of the real code? The size and the complexity of the method are relevant in terms of how much benefit you can extract from following design principles. Another thing: Don't forget, the flip side of "reduce coupling" is "increase cohesion" - components that are cohesive naturally experience more coupling within that cohesive unit compared to the loose coupling between such components (you cannot eliminate all coupling, you have to be strategic about it). Mar 19, 2021 at 21:48
  • @FilipMilovanović: The functions in the code are often several hundred lines long and do mutliple things that are just somewhat related. Mar 20, 2021 at 13:03

1 Answer 1

2

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()

1
  • Both observations are correct. 1) The lambdas can be improved by passing in the context, which makes them independent, and testable. 2) That using std::transform improves the code by moving from an explicit loop to a call into an algorithm that someone else, someone pretty reliable, tested exhaustively - so you know it's going to work. What's left for it is so trivial the testing of it is also trivial.
    – davidbak
    Mar 19, 2021 at 16:49

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.