HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Terminal ESC Sequence Decoder

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
decoderterminalescsequence

Problem

The goal of this code is to read-in a stream of terminal ESC sequences, and break them down and fill a vector of parameterized sequences for processing later on.

For example ESC[5C would be filled in the vector as:

Vector[0] = C    
Vector[1] = 5


Colors for example ESC[0;44;32m

Vector[0] = m
Vector[1] = 0
Vector[2] = 44
Vector[3] = 32


This way I can always check vector [0] for the control character, then the following for it's separated parameters.

Most of the Xterm sequences are not yet supported, but their sequences are noted in the case statements for later implementation.

I use a state machine with processing levels to move along each sequences
each level should move to the next expected control character.

Just looking for some general feedback on design or if anyone notices any issues or oversights on my part.

sequence_decoder.hpp

```
#ifndef SequenceParser_H
#define SequenceParser_H

#include "safe_queue.hpp"
#include "message_queue.hpp"

#include
#include

#include
#include

// Forward Deceleration
class Session;
typedef boost::shared_ptr session_ptr;
typedef boost::weak_ptr session_weak_ptr;

/**
* @class SequenceDecoder
* @author Michael Griffin
* @date 12/20/2015
* @file sequence_decoder.hpp
* @brief Processes incoming data into a message queue.
*/
class SequenceDecoder
{
public:

SequenceDecoder(session_ptr session);
~SequenceDecoder();

/**
* @brief Queue the Pasred Data back to the Session for Display
* @return
*/
bool sessionQueue();

// {Main Execution Method}
// Validate and Decode ESC Sequences
void decodeEscSequenceData(std::string &input_string);

private:

// Handle to Session for Sending Responses to Client.
session_weak_ptr m_weak_session;

// Holds Individual Sequences
MessageQueue m_message_queue;

// Sequence Parser State
enum
{
SEQ_NORMAL = 0, // Normal Text Data
SEQ_START = 1, // Start of ESC Seq

Solution

I see a number of things that may help you improve your code. First, though, let me say that the comments are excellent and greatly ease understanding the code. Keep that up!

Simplify the parser

The parser uses large switch statements and multiple named level parsers to decode. While the code is probably reasonably efficient (most of the bulk is comments), it does make things harder to read and maintain. An alternative approach would be to move from a code-centric to a data-centric model. In particular, if your compiler supports C++11, it seems that a std::unordered_set would be a good match. If, for some reason, your compiler does not support such constructs, you could build your own trie structure.

Another option would be to re-examine the various switch statements. For example, the 354-line processSequenceLevel1() function could be replaced with this equivalent code:

void SequenceDecoder::processSequenceLevel1()
{
    static const std::string valid{"@ABCDEFGHIJKLMPSTXZ`abcdefghilmnpqrstux!"};
    static const std::string nextlevel("? "};
    if(m_sequence == ';' || std::isdigit(m_sequence)) {
        return;
    }
    if (std::string::npos != valid.find(m_sequence) {
        m_is_sequence_completed = true;
    } else if (std::string::npos != nextlevel.find(m_sequence) {
        ++m_sequence_level;
    } else {
        m_is_invalid_sequence = true;
    }
}


Reduce the scope of variables

There are a number of member variables that do not really need to exist. For example, m_is_sequence could instead be a local variable within decodeEscSequenceData instead of a member value. This makes the object less complex and the code easier to understand and maintain.

Remember to unlock resources

We don't have the code for session.hpp but in several points in the code we see code like this:

session_ptr session = m_weak_session.lock();


However, there does not appear to be a matching unlock() call anywhere. Unless this is implicitly handled by the session object, that could be a problem.

Code Snippets

void SequenceDecoder::processSequenceLevel1()
{
    static const std::string valid{"@ABCDEFGHIJKLMPSTXZ`abcdefghilmnpqrstux!"};
    static const std::string nextlevel("? "};
    if(m_sequence == ';' || std::isdigit(m_sequence)) {
        return;
    }
    if (std::string::npos != valid.find(m_sequence) {
        m_is_sequence_completed = true;
    } else if (std::string::npos != nextlevel.find(m_sequence) {
        ++m_sequence_level;
    } else {
        m_is_invalid_sequence = true;
    }
}
session_ptr session = m_weak_session.lock();

Context

StackExchange Code Review Q#119165, answer score: 2

Revisions (0)

No revisions yet.