How to stay DRY with return values

Question

I have a bunch of repetitive C++ code that looks like this:

// Compute finalOutput if possible. Return true if successful, false otherwise
// finalOutput only holds a valid value if true is returned.
bool getCompositeValue(double& finalOutput) {
  double output_1 = 0
  bool success_1 = hopefullyGetSomeData("Data_1", output_1);  //output_1 is passed by reference
  if(!success_1) {
    logAnError("Couldn't get the first datum");
    return false;
  }

  double output_2 = 0;
  bool success_2 = hopefullyGetSomeData("Data_2", output_2);
  if(!success_2) {
    logAnError("Couldn't get the second datum");
    return false;
  }

  // ...
  finalOutput = output_1 - output_2 * output_3; //Some formula here
  return true;
}

How can I refactor this code to be more DRY? My first instinct is to use macros, but my second instinct is that this situation isn't special enough to justify the terribleness of macros in C++.

This is not a duplicate of Approaches to checking multiple conditions? because the answer to that question (specifically, approach #3 of the accepted answer) basically suggested the code style that I'm using in the my unsatisfactory code.

Possible duplicate of [Approaches to checking multiple conditions?](http://programmers.stackexchange.com/questions/191208/approaches-to-checking-multiple-conditions) — gnat, Apr 06 '16 at 18:01
Is the code for `hopefullyGetSomeData()` open to modification? Or is it part of some external API? — Julian, Apr 06 '16 at 19:05

5gon12eder · Accepted Answer · 2016-04-07T04:27:25.633

This could be refactored in several ways with varying levels of elegance and intrusiveness.

Do repetitive things in a loop

The idea is simple: Put your values into arrays and use a loop to retrieve them.

void
logAnError(const std::string& message);

bool
hopefullyGetSomeData(const std::string& id, double& destination);

bool
getCompositeValue(double& finalOutput)
{
  using tuple_type = std::tuple<std::string, double>;
  tuple_type parameters[] = {
    tuple_type {"Data_1", NAN},
    tuple_type {"Data_2", NAN},
    tuple_type {"Data_3", NAN},
  };
  for (auto& param : parameters)
    {
      const auto success = hopefullyGetSomeData(std::get<0>(param),
                                                std::get<1>(param));
      if (!success)
        {
          logAnError("Could not get item: " + std::get<0>(param));
          return false;
        }
    }
  finalOutput = std::get<1>(parameters[0])
    - std::get<1>(parameters[1]) * std::get<1>(parameters[2]);
  return true;
}

This refactoring is local to the getCompositeValue function. This might be what you're currently looking after but it also means that the more fundamental problems cannot be solved.

Use exceptions instead of error codes to communicate failure

If you're willing to touch the definition of the hopefullyGetSomeData function to throw an exception if it cannot return a value, your code could become much cleaner.

void
logAnError(const std::string& message);

// Throws exception on failure to retrieve the value.
double
hopefullyGetSomeData(const std::string& id);

// Does not throw exceptions.
bool
getCompositeValue(double& output)
{
  try
    {
      const auto out1 = hopefullyGetSomeData("Data_1");
      const auto out2 = hopefullyGetSomeData("Data_2");
      const auto out3 = hopefullyGetSomeData("Data_3");
      output = out1 - out2 * out3;
      return true;
    }
  catch (const std::exception& e)
    {
      logAnError(e.what());
      return false;
    }
}

But why stop there? If you can modify your callers as well, the code – and their code – becomes even cleaner and simpler.

// Throws exception on failure to retrieve the value.
double
getCompositeValue()
{
  const auto out1 = hopefullyGetSomeData("Data_1");
  const auto out2 = hopefullyGetSomeData("Data_2");
  const auto out3 = hopefullyGetSomeData("Data_3");
  return out1 - out2 * out3;
}

Use optional values

If you expect failure at a rate that is too high to make you feel comfortable using exceptions to communicate it, optional values can help. Unfortunately, the current proposal for adding them to the C++ standard library does not specify operator overloads so std::experimental::optional<T> cannot be used like a monad. Fortunately, we can provide these overloads ourselves.

// Do this for all operators you're interested.  I'm only showing `-` here.
// You'll need at least an overload for `*` as well to make the following
// example compile.

template <typename T>
std::enable_if_t<std::is_arithmetic<T>::value, std::experimental::optional<T>>
operator-(const std::experimental::optional<T>& lhs,
          const std::experimental::optional<T>& rhs)
{
  auto result = std::experimental::optional<T> {};
  if (lhs && lhs)
    result = lhs.value() - rhs.value();
  return result;
}

Now you can simply write your code like this.

std::experimental::optional<double>
hopefullyGetSomeData(const std::string& id);

std::experimental::optional<double>
getCompositeValue()
{
  const auto out1 = hopefullyGetSomeData("Data_1");
  const auto out2 = hopefullyGetSomeData("Data_2");
  const auto out3 = hopefullyGetSomeData("Data_3");
  return out1 - out2 * out3;
}

If you need early returns, you can add it like this,

// Helper function to reduce typing.
std::experimental::optional<double>
logAndReturnNothing(const std::string& message)
{
  logAnError(message);
  return std::experimental::optional<double> {};
}

std::experimental::optional<double>
getCompositeValue()
{
  const auto out1 = hopefullyGetSomeData("Data_1");
  if (!out1)
    return logAndReturnNothing("Cannot get first value");
  const auto out2 = hopefullyGetSomeData("Data_2");
  if (!out2)
    return logAndReturnNothing("Cannot get second value");
  const auto out3 = hopefullyGetSomeData("Data_3");
  if (!out3)
    return logAndReturnNothing("Cannot get third value");
  return out1.value() - out2.value() * out3.value();
}

which I find a pretty high price to pay as it basically trashes the gains from using optionals in the first place. Note that this last function doesn't need the overloads for handling arithmetic with optional<T>s. It would be better to log the error at the point where it actually occurs (ie, inside the hopefullyGetSomeData function) but early returns could be important for performance. Exceptions give you early “returns” for free.

Ideally, the language itself would support monadic expressions such that

return hopefullyGetSomeData("Data_1")
    - hopefullyGetSomeData("Data_2") * hopefullyGetSomeData("Data_3");

would short-circuit as soon as any of the operands is not available. But that's not what we have in C++ today.

+1: For mentioning optional and the possibility to use a monad. optional should be used instead of exceptions if failure is expected and can occur often. — Giorgio, Apr 07 '16 at 05:39

score 0 · Answer 2 · answered Apr 06 '16 at 19:54

An alternate approach is to return a struct which combines whether the value is valid and what the value is. (This is admittedly more natural in languages with multiple returns.) Like this:

struct maybe_double {
  bool isValid;
  double value;
};

maybe_double getCompositeValue() {
  maybe_double term1 = hopefullyGetSomeData("Data1");
  if (! term1.isValid) {
    logAnError("Couldn't get the first datum");
    return term1;
  }

  maybe_double term2 = hopefullyGetSomeData("Data2");
  if (! term2.isValid) {
    logAnError("Couldn't get the second datum");
    return term2;
  }

  maybe_double term3 = hopefullyGetSomeData("Data3");
  if (! term3.isValid) {
    logAnError("Couldn't get the third datum");
    return term3;
  }

  maybe_double answer;
  answer.isValid = true;
  answer.value = term1.value - term2.value * term3.value;
  return answer;
}

Now we can move the logging into hopefullyGetSomeData and simplify to:

struct maybe_double {
  bool isValid;
  double value;
};

maybe_double getCompositeValue() {
  maybe_double term1 = hopefullyGetSomeData("Data1");
  if (! term1.isValid) {
    return term1;
  }

  maybe_double term2 = hopefullyGetSomeData("Data2");
  if (! term2.isValid) {
    return term2;
  }

  maybe_double term3 = hopefullyGetSomeData("Data3");
  if (! term3.isValid) {
    return term3;
  }

  maybe_double answer;
  answer.isValid = true;
  answer.value = term1.value - term2.value * term3.value;
  return answer;
}

And now the code is so repetitive that we can easily turn it into a loop.

struct maybe_double {
  bool isValid;
  double value;
};

maybe_double getCompositeValue() {
  maybe_double terms [3];
  std::string names [3] = {"Data1", "Data2": "Data3"};
  for (0 == i; i < 3; i++) {
    maybe_double term = hopefullyGetSomeData(names[i]);
    if (term.isValid)
      terms[i] = term;
    else
      return term
  }

  maybe_double answer;
  answer.isValid = true;
  answer.value = terms[0].value - terms[1].value * terms[2].value;
  return answer;
}

How to stay DRY with return values

2 Answers2

Do repetitive things in a loop

Use exceptions instead of error codes to communicate failure

Use optional values