Much Virtue in If

In my last post I noted having the following function made possible without an if/else test against the iterator by having the following class as always the last one checked:

const LtLibrary::IAdditionDiscriminator &

AdditionDiscriminatorSet::findMatch(const IFilterableRecord &inNewRec) const

{

  return *(*(std::ranges::find_if(

               m_additionDiscriminators,

               [&](const auto &inVal) { return inVal->reject(inNewRec); })))

              .get();

}

class AlwaysAdditionDiscriminator : public LtLibrary::IAdditionDiscriminator

{

public:

  ~AlwaysAdditionDiscriminator() override {}


  bool reject(const LtLibrary::IFilterableRecord &inRecord) const override

  {

    return true;

  }

  LtLibrary::ILibraryBookRecord *

  returnRecord(LtLibrary::LibraryBookRecord &&inRec) const override

  {

    LtLibrary::LibraryBookRecord *rval

        = new LtLibrary::LibraryBookRecord(std::move(inRec));

    rval->setIsOnHeap(true);

    return rval;

  }

  LtLibrary::ILibraryBookRecord *

  returnRecordCopy(const LtLibrary::ILibraryBookRecord &inRec) const override

  {

    return inRec.clone();

  }

  std::string getType() const override

  {

    return "AlwaysAdditionDiscriminator";

  }

  void updateStats(LtLibrary::LtStats &outStats,

              const LtLibrary::BaseLibraryBookRecord &inRec) const override

  {

    outStats.add(inRec);

  }

};

The original code had been along the lines of

const LtLibrary::LibraryBookRecord*

AdditionDiscriminatorSet::findMatch(LtLibrary::LibraryBookRecord &&inRec) const

{

  auto iter = std::ranges::find_if(

m_additionDiscriminators,

               [&](const auto &inVal) { return inVal->reject(inRec); });

  if (iter == m_additionDiscriminators.end())

  {

    LtLibrary::LibraryBookRecord *rval

        = new LtLibrary::LibraryBookRecord(std::move(inRec));

    rval->setIsOnHeap(true);

    return rval;

  }

  else

  {

      // Return pointer to null object

  }

}

Now, it's obvious that the existing code is a little cleaner, and that is a good thing.  The code could have been taken in the opposite direction on refactoring, to:

const LtLibrary::LibraryBookRecord*

AdditionDiscriminatorSet::findMatch(LtLibrary::LibraryBookRecord &&inRec) const

{

  if (std::ranges::none_of(

m_additionDiscriminators,

               [&](const auto &inVal) { return inVal->reject(inRec); }))

  {

    LtLibrary::LibraryBookRecord *rval

        = new LtLibrary::LibraryBookRecord(std::move(inRec));

    rval->setIsOnHeap(true);

    return rval;

  }

  else

  {

      // Return pointer to null object

  }

}

which is also an acceptable refactoring in itself.  Why did I decide to go with the sentinel model?  Because the final code produces clearer and simpler logic at each level.

(Had the code been such as to avoid the else part of the test, needed to return a null object, I might have decided differently.  The longer the logic resulting from a test, the less easy it is to maintain.)

In general, anything which branches is to be avoided, if possible, unless it involves contortions.  Branches detract from clarity; they can also affect performance, and there's also a general structural problem, more relevant to what I'll be talking about below, which has been popularized by the phrase "if considered harmful": https://www.youtube.com/watch?v=z43bmaMwagI .

(You can probably ignore performance. Back in the day, avoiding jumps was critical to good performance; then we got modern processors with multiple pipelines and branch prediction, lookahead and ... they can still be important for performance, especially if the processor is busy and you are in a tight loop and the result isn't easy to project; you can have to throw away a whole block of precalculated results if you branch to an execution flow which is different. But the handling of branching has improved enough that none of this should be a performance concern at all unless you are in the middle of a tight loop in a critical area; compilers frequently convert apparent branches to nonbranching code. And virtual functions themselves can trigger cache misses and interfere with pipelining. Now, there are a ton of little if/else tests buried in a high-level std::find_if.  Every application of your predicate will result in an if/else branch; they just aren't visible at the code level.  Converting the explicit if/else of the earlier test does frequently save on one test but in the overall context the saving is minuscule. There won't be a significant performance effect no matter what.

If you have to branch in a time-critical area, think about what the performance profile is likely to be.  You may find use for one of the new C++20 attributes:

if (std::size_t index = s.find('_'); index != std::string::npos) [[likely]]

...

)

The final solution above isolates the logic in the sentinel class and then simplifies the overall flow. It also segregates the logic into two parts (find the appropriate discriminator and call the logic to store the final result) while making the syntax in both places clearer. Removing if tests won't make your code wildly more efficient.  But it has enough other benefits to be worth making into a policy, and in most cases the efficiency question isn't likely to be significant. 

Consider another refactoring I recently did.

I had a boolean flag controlling the display of a title.  It got checked for every record being displayed, and led to the choice of one of two gtk calls.  I replaced it with a stateless ITitleInserter class, had two static variants, one corresponding to each branch, and instead of setting the boolean flag I reset a pointer between the two implementations.  The call site became clearer, the logic was better encapsulated, and the if/else test was entirely eradicated.

  class ITitleInserter

  {

  public:

    virtual ~ITitleInserter();


    virtual ITitleInserter *insertTitle(const std::string &inTitle,

                                        GtkTextBuffer *inBuffer,

                                        GtkTextIter *inIter) const = 0;


    virtual void insertPostTitle(const std::string &inStr,

                                 GtkTextBuffer *inBuffer,

                                 GtkTextIter *inIter) const = 0;

  };

implemented as:

class NormalTitleInserter

    : public GtkGridBibliographicFieldFormatter::ITitleInserter

{

public:

  ~NormalTitleInserter() override {}

  ITitleInserter *insertTitle(const std::string &inTitle,

                              GtkTextBuffer *inBuffer,

                              GtkTextIter *inIter) const override;

  void insertPostTitle(const std::string &inStr, GtkTextBuffer *inBuffer,

                       GtkTextIter *inIter) const override

  {

    if (!inStr.empty())

      gtk_text_buffer_insert(inBuffer, inIter, inStr.c_str(), inStr.length());

  }

};

NormalTitleInserter theNormalInserter;

auto NormalTitleInserter::insertTitle(const std::string &inTitle,

                                 GtkTextBuffer *inBuffer,

                                 GtkTextIter *inIter) const -> ITitleInserter* 

{

  gtk_text_buffer_insert_with_tags_by_name(

      inBuffer, inIter, inTitle.c_str(), inTitle.length(), "italic", nullptr);

  return &theNormalInserter;

}


class UnderReconsiderationTitleInserter

    : public GtkGridBibliographicFieldFormatter::ITitleInserter

{

public:

  ~UnderReconsiderationTitleInserter() override {}

  ITitleInserter *insertTitle(const std::string &inTitle,

                              GtkTextBuffer *inBuffer,

                              GtkTextIter *inIter) const override

  {

    gtk_text_buffer_insert_with_tags_by_name(inBuffer, inIter, inTitle.c_str(),

                                             inTitle.length(), "italic",

                                             "error", nullptr);

    return &theNormalInserter;

  }

  void insertPostTitle(const std::string &inStr, GtkTextBuffer *inBuffer,

                       GtkTextIter *inIter) const override

  { }

};

and called as:

      m_inserter

          = m_inserter->insertTitle(s.substr(0, index), m_buffer, &m_iter);

      m_inserter->insertPostTitle(s.substr(index + 1), m_buffer, &m_iter);

The advice given in the "if considered harmful" discussion goes beyond that: pass around classes corresponding to different possible states of your data or environment, and execute them via virtual functions.  This decreases the likelihood of bugs under maintenance.

... and, in fact, this story ends up having that aspect as well -- because I then found another class using the same logic for formatting titles in a different GTK context.  So this trio of classes was hoisted to more general use, and -- should the requirements change -- there is now one place, not two places, for the logic to change.

Comments

Post a Comment

Popular posts from this blog

Boundaries

Imagine that you have a well-designed application, possibly using dependency injection. It will almost always have a very thin layer visible in main() - capturing command-line parameters and environment variables, mainly[1], and then a call to an Application class which manages the rest of the initial logic - setting up resources such as logging, doing substantive validation of parameters, and creating and running the objects on the next level down which actually carry out the work of the application. This is simply following the SRP at a high level.
Read more

State Machines

Almost any program can be modelled as a state machine. I say "almost any" because it is, just, possible to think of a program which cannot easily be resolved into anything more than a one-state state machine. Consider a command interpreter with no support for loops or function definitions (like the old DOS command.com) and with no internal commands, only the ability to execute external commands. It loops forever and does only one thing for every line it reads.    If it does no error handling, the execution is a single line, a call to execve or one of its relatives. There are internal stages inside the C library call, but they are effectively invisible to the program.  However, it's not a very useful program. Neither is any version of HelloWorld. The echo application does have its uses. Once one gets beyond this, even simple programs have at least two states: setup and execute. A simple version of cp has to open two files for input and output before beginning to copy data....
Read more

Considerations on an Optimization

An application – actually, successive pair of applications – is meant to process (in the end) about three hundred or so entries to convert a simple markup into complex LaTeX markup. It also enhances the data with information drawn from a flat-file text database (i.e. literally a set of .txt files in a single directory). This application was originally written in rust, which is not my favourite language, but one worth exercising to a degree. The rust application covered a limited subdomain, and generally avoided unsafe rust code, although in some cases it had to use cell mechanisms which converted compile-time to runtime safety. The initial iteration had the text database as one large file, with some logic allowing for extended seeking through chapters. It soon became apparent, after extending my dataset to about twenty or so items, that this was going to result in an unacceptable runtime — it was taking longer to process than the later TeX processing took, and although in theory this w...
Read more