A Small Refactoring

 In my last post I finished up by saying:

"The duplication in pattern in setting the two fields, above, involves such a small range that the extra overhead would be as much of a problem as the duplicated logic it encapsulated."

Of course, I had no sooner posted the above than I went and looked at the broader codebase and found (1) an exact repetition of the pattern in another class and (2) a near repetition of the pattern in a third class.

The third class is, in fact, different enough that extracting commonality would have been more difficult (and it has also accordingly found a different solution to its similar problem, to boot).  But the second class, which is a parent class for simpler Versicle/Response pairs had pretty well exactly the same logic.

This is not a case where extracting common logic via inheritance was the best choice; in fact, it rarely is.  Usually, you get both clearer code and more flexibility by delegation.

I had a bare array of two strings modelling the two lines.  They were set in the following way:

{

    VersicleElement e(rest);

    rest.remove_prefix(incrementLength(e.getLength()));

    m_vals[0] = e.getText();

  }

  rest.remove_prefix(compareActualWithExpectedIndex(

      rest, getNextTagIndex(rest), incrementOverWhitespace(rest)));

  {

    ResponseElement e(rest);

    rest.remove_prefix(incrementLength(e.getLength()));

    m_vals[1] = e.getText();

  }

Different objects used for extraction, different indexes, the rest was identical.  The main constraint is that this should be simple and fairly cheap, as the weakness it is fixing was a minor one,

Turning that bare array into a class was the first step.  Not only does it need to support incremental updating in this way, but also through simply copying two values in from a collection.

class VersicleAndResponsePair

{

public:

  void extract(std::string_view &ioText, MultiElementElement &outParent);

  void copy(std::span<std::string> inVals);

  std::span<std::string> getValues() const { return m_vals; }

private:

  mutable std::array<std::string, 2> m_vals;

  void extractOne(const int inIndex, std::string_view &ioText,

                  MultiElementElement &outParent);

};

This is deliberately limited in its scope.  There's no attempt to make a single class to support formatting V/R pairs.  (For one thing, most V/R formatting uses hardcoded arguments, not arrays, and secondly I have tended to keep the two domains distinct (printable office representations which have few dependencies on the concrete details of how they are stored and and non-printable classes which manage the assembly of the data).  This was not a place to blur them.)

The methods are short, simple, and designed to adhere to the DRY principle:

void VersicleAndResponsePair::extract(std::string_view &ioText,

                                      MultiElementElement &outParent)

{

  extractOne(0, ioText, outParent);

  outParent.adjustBetweenTags(ioText);

  extractOne(1, ioText, outParent);

}

void VersicleAndResponsePair::copy(std::span<std::string> inVals)

{

  if (inVals.size() > 1)

    std::copy_n(inVals.begin(), 2, m_vals.begin());

}

void VersicleAndResponsePair::extractOne(const int inIndex,

                                         std::string_view &ioText,

                                         MultiElementElement &outParent)

{

  auto l = [&](const SimpleTextElement &inElement) -> void {

    ioText.remove_prefix(outParent.incrementLength(inElement.getLength()));

    m_vals[inIndex] = inElement.getText();

  };

  if (inIndex == 0)

    l(VersicleElement(ioText));

  else

    l(ResponseElement(ioText));

}

Using this instead of the original array, and with a convenience function added to MultiElementElement to cover the messy call, the original code:

void HymnResponsesElement::processBody(std::string_view inText)

{

  std::string_view rest(inText.substr(getLength()));

  rest.remove_prefix(compareActualWithExpectedIndex(

      rest, getNextTagIndex(rest), incrementOverWhitespace(rest)));

  {

    VersicleElement e(rest);

    rest.remove_prefix(incrementLength(e.getLength()));

    m_vals[0] = e.getText();


  }

  rest.remove_prefix(compareActualWithExpectedIndex(

      rest, getNextTagIndex(rest), incrementOverWhitespace(rest)));

  {

    ResponseElement e(rest);

    rest.remove_prefix(incrementLength(e.getLength()));

    m_vals[1] = e.getText();

  }

  rest.remove_prefix(compareActualWithExpectedIndex(

      rest, getNextTagIndex(rest), incrementOverWhitespace(rest)));

  if (!rest.starts_with(getEndTag()))

    throw OfficeParseException(

        getStartTagName() + " with unexpected element before closing tag",

        rest);

  incrementLength(getEndTag().length());

}

Becomes reduced to:

void HymnResponsesElement::processBody(std::string_view inText)

{

  std::string_view rest(inText.substr(getLength()));

  adjustBetweenTags(rest);

  m_vals.extract(rest, *this);

  adjustBetweenTags(rest);

  if (!rest.starts_with(getEndTag()))

    throw OfficeParseException(

        getStartTagName() + " with unexpected element before closing tag",

        rest);

  incrementLength(getEndTag().length());

}

and the same changes affect the other class as well.  The overhead involved to fix the one location was disproportionate, but the application to two locations makes it worthwhile.

The existing unit tests cover the calling class (which fully exercises the new class for its core functionality) and confirm that its visible behaviour is unchanged.

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