Alternative control flows to highlight happy-path

Question

Moved from stackoverflow

Often when writing code I need to check return values before continuing with the operation. This means that the code is cluttered by control-flow that I don't really care about, and that following the happy-flow is harder for readers of the code in the future.

Apart from early returns and nesting if-statements, is there a way I can organize my function's body to make the intent of the happy-path clearer, and draw less attention to the handling of edge-cases?

I can't use exceptions, so relying on the underlying calls throwing isn't a possibility either.

For illustration, here is a function that tries to encode some data, and write it to a file, written in a style with early returns.

bool ComponentVersionDevice::CommitCurrentBaseline() {
  FileHandle baseline{(&m_fs), FilesystemPartition_e::CCONFIGURATION_PARTITION};
  if (!baseline.OpenFile(CURRENT_BASELINE_LOCATION)) {
    return false;
  }
  JsonEncoder encoder;
  m_parseBuffer.clear();
  const auto lengthOrErrorCode =
      encoder.Encode(m_currentBaseline, m_parseBuffer.begin(), m_parseBuffer.max_size(), OBJECT);
  if (lengthOrErrorCode < 0) {
    LOGGER.WriteE(LOG_TAG, "Couldn't encode the current baseline. JSON error: [%d]", lengthOrErrorCode);
    return false;
  }
  if (lengthOrErrorCode == PARSE_BUFFER_SIZE || m_parseBuffer.truncated() != 0) {
    LOGGER.WriteE(LOG_TAG, "Couldn't encode the current baseline, it's too large for our parsing buffer...");
    return false;
  }
  if (!baseline.Write(m_parseBuffer)) {
    LOGGER.WriteE(LOG_TAG, "Couldn't write the encoded currentbaseline to the file.");
    return false;
  }
  return true;
}

Almost the same code written using nested if's:

bool ComponentVersionDevice::CommitCurrentBaseline() {
  FileHandle baseline{(&m_fs), FilesystemPartition_e::CCONFIGURATION_PARTITION};
  if (baseline.OpenFile(CURRENT_BASELINE_LOCATION)) {
    JsonEncoder encoder;
    m_parseBuffer.clear();
    const auto lengthOrErrorCode =
        encoder.Encode(m_currentBaseline, m_parseBuffer.begin(), m_parseBuffer.max_size(), OBJECT);
    if(lengthOrErrorCode > 0){
      if (!m_parseBuffer.truncated()) {
        if (baseline.Write(m_parseBuffer)) {
          return true;
        } else {
          LOGGER.WriteE(LOG_TAG, "Couldn't write to file");
        }
      } else {
        LOGGER.WriteE(LOG_TAG, "Encoded baseline doens't fit in buffer!");
      }
    } else {
      LOGGER.WriteE(LOG_TAG, "Couldn't encode, error: %d", lengthOrErroCode);
    }
  }
  return false;
}

What the actual code does, without error handling:

bool ComponentVersionDevice::CommitCurrentBaseline() {
  FileHandle baseline{(&m_fs), FilesystemPartition_e::CCONFIGURATION_PARTITION};
  baseline.OpenFile(CURRENT_BASELINE_LOCATION)
  JsonEncoder encoder;
  m_parseBuffer.clear();
  encoder.Encode(m_currentBaseline, m_parseBuffer.begin(), m_parseBuffer.max_size(), OBJECT);
  baseline.Write(m_parseBuffer);
  return true;
}

Here you can more clearly see what the intent is, and what the function is supposed to do.

Is there another way to chain operations that have dependencies on the previous steps succeeding without having all the error handling interwoven like in the examples?

Some technical constraints given the environment I want to apply this to:

• No exceptions
• Little or preferably no heap usage
• using features available in c++20 or earlier

Answer 1

Some suggestions:

Write code in paragraphs. Group related lines, and put empty lines between those groups in the same way that English text is written.

---

If possible, avoid two stage initialization:

FileHandle baseline{(&m_fs), FilesystemPartition_e::CCONFIGURATION_PARTITION};
if (!baseline.OpenFile(CURRENT_BASELINE_LOCATION)) {
  return false;
}

might be better written as

auto baseline = FileHandle(&m_fs, FilesystemPartition_e::CCONFIGURATION_PARTITION, CURRENT_BASELINE_LOCATION);

if (!baseline.IsOpen())
{
  LOGGER.WriteE(LOG_TAG, "Couldn't open the current baseline file for writing.");
  return false;
}

---

Don't hide operations inside if statements:

if (!baseline.Write(m_parseBuffer)) {
  LOGGER.WriteE(LOG_TAG, "Couldn't write the encoded currentbaseline to the file.");
  return false;
}

Could be:

auto const writeResult = baseline.Write(m_parseBuffer);

if (!writeResult) {
  LOGGER.WriteE(LOG_TAG, "Couldn't write the encoded currentbaseline to the file.");
  return false;
}

This preserves a simple pattern - do the operation, check the result. Terse code isn't always easier to read.

---

So, perhaps:

bool ComponentVersionDevice::CommitCurrentBaseline()
{
    // encode
    {
        m_parseBuffer.clear();

        auto encoder = JsonEncoder();
        auto const encodeResult = encoder.Encode(m_currentBaseline, m_parseBuffer.begin(), m_parseBuffer.max_size(), OBJECT);

        if (encodeResult < 0)
        {
            LOGGER.WriteE(LOG_TAG, "Couldn't encode the current baseline. JSON error: [%d]", encodeResult);
            return false;
        }

        if (encodeResult == PARSE_BUFFER_SIZE || m_parseBuffer.truncated() != 0)
        {
            LOGGER.WriteE(LOG_TAG, "Couldn't encode the current baseline, it's too large for our parsing buffer...");
            return false;
        }
    }

    // write
    {
        auto baseline = FileHandle(&m_fs, FilesystemPartition_e::CCONFIGURATION_PARTITION, CURRENT_BASELINE_LOCATION);

        if (!baseline.IsOpen())
        {
            LOGGER.WriteE(LOG_TAG, "Couldn't open the current baseline file for writing.");
            return false;
        }

        auto const writeResult = baseline.Write(m_parseBuffer);

        if (!writeResult)
        {
            LOGGER.WriteE(LOG_TAG, "Couldn't write the encoded current baseline to the file.");
            return false;
        }
    }

    return true;
}

---

You could look into some sort of Result<> class like std::expected. But without Zig-style "try" syntax in C++, it's much the same as just checking for errors in the normal way.

Answer 2

Are you open to extensions?

One feature that standard C++ critically lacks is a way to create expressions that can embed control-flow instructions in a practical manner. GCC solves the issue elegantly with its Statement Exprs extension, also called "Compound Statements", which are widely used in the Linux kernel (no exception in C, either).

I'll use std::expected in this example. Being C++23 only, you'll need an alternative implementation, but for the purpose of this example it'll suffice.

The crux of the solution is the following macro:

#define TRY(...)                                                    \
({                                                                  \
    auto expected = (__VA_ARGS__);                                  \
    if (!expected.has_value()) {                                    \
        return std::unexpected(expected.error());                   \
    }                                                               \
    expected.value();                                               \
})

This macro uses the Compound Statement extension of GCC to create an expression which may actually return from the current function:

• If the std::expected<T, E> argument contains a value, then the expression evaluates to this value.
• Otherwise, the expression returns from the current function with the error embedded, which may be converted.

Based on that, and assuming that the functions you call do use std::expected, then your code can be written as:

auto ComponentVersionDevice::CommitCurrentBaseline() -> std::expected<void, char const*> {
    FileHandle baseline{(&m_fs), FilesystemPartition_e::CCONFIGURATION_PARTITION};

    TRY(baseline.OpenFile(CURRENT_BASELINE_LOCATION));

    JsonEncoder encoder;
    m_parseBuffer.clear();

    const auto length =
        TRY(encoder.Encode(m_currentBaseline, m_parseBuffer.begin(), m_parseBuffer.max_size(), OBJECT));

    if (length == PARSE_BUFFER_SIZE || m_parseBuffer.truncated() != 0) {
        return std::unexpected("Couldn't encode the current baseline, it's too large for our parsing buffer...");
    }
    
    TRY(baseline.Write(m_parseBuffer));
    
    return {};
}

Or perhaps even shorter, since arguably the check that on the length of the parse buffer should be done inside the Encode method, but I wanted to show how to return an error :)

I would arguable this is fairly readable.

Even a longer macro name (TRY_OR_BAIL?) would not hinder readability.

Answer 3

Just to add some more c++ style on the way the 17 version.

Instead of

auto const encodeResult = encoder.Encode(m_currentBaseline, m_parseBuffer.begin(), m_parseBuffer.max_size(), OBJECT);

if (encodeResult < 0)
{            
    LOGGER.WriteE(LOG_TAG, "Couldn't encode the current baseline. JSON error: [%d]", encodeResult);
    return false;
}

You can transform to

if (auto const encodeResult = encoder.Encode(m_currentBaseline, m_parseBuffer.begin(), m_parseBuffer.max_size(), OBJECT); encodeResult < 0)
{    
    LOGGER.WriteE(LOG_TAG, "Couldn't encode the current baseline. JSON error: [%d]", encodeResult);
    return false;
}

Answer 4

I use the following style for medical C code, where multiple returns and goto are forbidden:

bool ComponentVersionDevice::CommitCurrentBaseline() {
  FileHandle baseline{(&m_fs), FilesystemPartition_e::CCONFIGURATION_PARTITION};
  bool success = baseline.OpenFile(CURRENT_BASELINE_LOCATION));

  if (success)
  {
    JsonEncoder encoder;
    m_parseBuffer.clear();
    const auto lengthOrErrorCode =
        encoder.Encode(m_currentBaseline, m_parseBuffer.begin(), m_parseBuffer.max_size(), OBJECT);

    if (lengthOrErrorCode < 0) {
      LOGGER.WriteE(LOG_TAG, "Couldn't encode the current baseline. JSON error: [%d]", lengthOrErrorCode);
      success = false;
    }
    else if (lengthOrErrorCode == PARSE_BUFFER_SIZE || m_parseBuffer.truncated() != 0) {
      LOGGER.WriteE(LOG_TAG, "Couldn't encode the current baseline, it's too large for our parsing buffer...");
      success = false;
    }
  }

  if (success && !baseline.Write(m_parseBuffer)) {
    LOGGER.WriteE(LOG_TAG, "Couldn't write the encoded currentbaseline to the file.");
    success = false;
  }

  // Or...

  if (success)
  {
    success = baseline.Write(m_parseBuffer);
    if (!success)
    {
        LOGGER.WriteE(LOG_TAG, "Couldn't write the encoded currentbaseline to the file.");
    }
  }

  return success;
}

Essentially, the pattern is:

success = ...

if (success)
{
  ...
  success = ...
} 

if (success)
{
  ...
  success = ...
}

return success;

This avoids problems with nesting, problems with multiple returns, and tends to generate quite nice assembly (the compiler's smart enough to just skip to the end of the function if success becomes false, rather than doing a lot of redundant checks).

Reading the happy path takes a little bit of getting used to, but it's generally "the thing after the next if (success) check", so you can scan down the function, using those if statements as reference points.