LTProject: Sorting; a Small Refactoring

 In the last post, I referenced the following function:

bool TitleRecordSortStrategy::isLessThan(

    const IMinimalLibraryBookRecord &inRec1,

    const IMinimalLibraryBookRecord &inRec2) const

{

  std::string_view s1 = inRec1.getTitle();

  std::string_view s2 = inRec2.getTitle();


  if (s1.empty())

    {

      return !s2.empty();

    }

  else

    {

      if (s2.empty())

        return false;

      else

        {

          int offset1 = inRec1.getTitleSortOffset();

          if ((offset1 > 0)

              && (s1.length() > static_cast<std::size_t>(offset1)))

            s1.remove_prefix(offset1);

          int offset2 = inRec2.getTitleSortOffset();

          if ((offset2 > 0)

              && (s2.length() > static_cast<std::size_t>(offset2)))

            s2.remove_prefix(offset2);

          try

            {

              return s1 < s2;

            }

          catch (const std::system_error &e)

            {

              std::ostringstream str;

              str << e.what() << ":" << s1 << "," << offset1 << "; " << s2

                  << "," << offset2;

              m_logger.log(Log::Severity::Fatal, str.str(), e.code());

              return false;

            }

          catch (const std::exception &ex)

            {

              std::ostringstream str;

              str << ex.what() << ":" << s1 << "," << offset1 << "; " << s2

                  << "," << offset2;

              m_logger.log(Log::Severity::Fatal, str.str());

              return false;

            }

        }

    }

}

And observed: "Most of the space in the implementation is taken up with error handling ... The error handling not only takes up the space in the catch blocks but drives the unrolling of the title offset logic, so that all the values are available for logging."  The repetition of the unrolled title adjustments violated the DRY principle and the space taken up by the exception reporting obscured the basic flow of logic.

I realized, shortly after, that moving both into one assisting class would improve the presentation in the main class, and done right could avoid much of the duplication.  As l'esprit de l'escalier is not a lost opportunity in software development -- you can always go back and improve the code -- here's the small refactoring.

Create a new inner class for handling the offsets,  Variables which have to be logged in the event of an exception are class members.

  class OffsetAdjuster

  {

  public:

    OffsetAdjuster(const Log::ILogger &inLogger) : m_logger(inLogger) {}

    std::string_view adjustFirst(std::string_view ioField,

                                 const IMinimalLibraryBookRecord &inRec);

    std::string_view adjustSecond(std::string_view ioField,

                                  const IMinimalLibraryBookRecord &inRec);

    void logSystemError(const std::system_error &inError) const;

    void logGeneralException(const std::exception &inError) const;

  private:

    int m_first = 0;

    int m_second = 0;

    std::string_view m_view1;

    std::string_view m_view2;

    const Log::ILogger &m_logger;

    int adjustField(std::string_view &ioField,

                    const IMinimalLibraryBookRecord &inRec);

    std::string buildExceptionString(const std::exception &ex) const;

  };

The two private methods capture the repeated code:

int TitleRecordSortStrategy::OffsetAdjuster::adjustField(std::string_view &ioField, const IMinimalLibraryBookRecord &inRec)

{

  int rval = inRec.getTitleSortOffset();

  if ((rval > 0) && (ioField.length() > static_cast<std::size_t>(rval)))

    ioField.remove_prefix(rval);

  return rval;

}

std::string TitleRecordSortStrategy::OffsetAdjuster::buildExceptionString(const std::exception &ex) const

{

  std::ostringstream str;

  str << ex.what() << ":" << m_view1 << "," << m_first << "; " << m_view2

      << "," << m_second;

  return str.str();

}

Leaving the public methods with simple implementations:

std::string_view TitleRecordSortStrategy::OffsetAdjuster::adjustFirst(std::string_view ioField, const IMinimalLibraryBookRecord &inRec)

{

  m_first = adjustField(ioField, inRec);

  m_view1 = ioField;

  return m_view1;

}

std::string_view TitleRecordSortStrategy::OffsetAdjuster::adjustSecond(std::string_view ioField, const IMinimalLibraryBookRecord &inRec)

{

  m_second = adjustField(ioField, inRec);

  m_view2 = ioField;

  return m_view2;

}

void TitleRecordSortStrategy::OffsetAdjuster::logSystemError(const std::system_error &inError) const

{

  m_logger.log(Log::Severity::Fatal, buildExceptionString(inError),

               inError.code());

}

void TitleRecordSortStrategy::OffsetAdjuster::logGeneralException(const std::exception &inError) const

{

  m_logger.log(Log::Severity::Fatal, buildExceptionString(inError));

}

This still is not an ideal expression of the SRP: in principle a separate class should have responsibility for the logging from that which is responsible for the tweaking of the title keys -- but it's a lot closer than it was.  (A separate class is certainly doable, but it would look very much like the buildExceptionString() function except with four extra variables being passed in.  In this case I'm going to define the single responsibility as "manage the variables around title sort offset handling" and leave it there.)

The effect on the original function is a significant improvement in clarity:

bool TitleRecordSortStrategy::isLessThan(

    const IMinimalLibraryBookRecord &inRec1,

    const IMinimalLibraryBookRecord &inRec2) const

{

  std::string_view s1 = inRec1.getTitle();

  std::string_view s2 = inRec2.getTitle();

  if (s1.empty())

    {

      return !s2.empty();

    }

  else

    {

      if (s2.empty())

        return false;

      else

        {

  OffsetAdjuster adjuster(m_logger);

          try

            {

              return adjuster.adjustFirst(s1, inRec1) < adjuster.adjustSecond(s2, inRec2);

            }

          catch (const std::system_error &e)

            {

      adjuster.logSystemError(e);

              return false;

            }

          catch (const std::exception &ex)

            {

      adjuster.logGeneralException(ex);

              return false;

            }

        }

    }

}

Note that the adjuster cannot be made a member object because one of the design aims of the sort strategies is that they be stateless, and the OffsetAdjuster carries state.  Instead it is instantiated in the smallest scope which is required for its functionality.

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