Manchester encoder/decoder

Question

I have written a Manchester Encoding encoder / decoder based on my misconception of how it works (whoops). My encoder accepts an array of ones and zeroes, and returns the 'manchester encoded' data (pretending there is a constant clock to overlay onto the data). I am reasonably new to C++, and would like to advance my knowledge and coding skill in it, hence why I coded this small application. I am looking for ways to improve my code, as well as ways to increase its efficiency.

Main.cpp

#include <iostream>
#include "Manchester.h"

int main()
{
    int data[] = { 1, 1, 0, 0 }; // Some unencoded data
    int* encoded = Manchester::encode(data, 4);
    int* decoded = Manchester::decode(encoded, 8);
    return 0;
}

Manchester.cpp

#include "Manchester.h"
#include <stdexcept>
#include <sstream>
#include <cstring>
#include <cstdlib>


#ifdef DEBUG
    #include <iostream>
#endif

int *Manchester::encode(int *data, int length)
{
    int *output = new int[length * 2];

    for (int i = 0; i < length; i++)
    {

        // Work out the array indexes to use
        int bid = i * 2;
        int nbid = bid + 1;

        // Get the data
        int real = data[i];

        int bit = 0;
        int nbit = 0;

        // Work out what it is
        switch (real)
        {
        case 0:
            bit = MANCHESTER_ZERO[0] - '0'; // Subtract 48 to work out the real value
            nbit = MANCHESTER_ZERO[1] - '0';
            break;
        case 1:
            bit = MANCHESTER_ONE[0] - '0'; // Subtract 48 to work out the real value
            nbit = MANCHESTER_ONE[1] - '0';
            break;
        }

        #ifdef DEBUG
            std::cout << "[encode] " << real << " [" << bit << nbit << "]" << std::endl;
        #endif

        output[bid] = bit;
        output[nbid] = nbit;
    }

    return output;
}

int *Manchester::decode(int *data, int length)
{
    if ((length % 2) != 0)
    {
        throw std::range_error("length is not a multiple of 2");
    }

    int *output = new int[length / 2];

    for (int i = 0; i < (length / 2); i++)
    {
        // Work out the array indexes to use
        int bid = i * 2;
        int nbid = bid + 1;

        // Get the data
        int bit = data[bid];
        int nbit = data[nbid];

        // Put the data into a stringstream for comparison
        std::stringstream con;
        con << bit << nbit;
        const char* sbit = con.str().c_str();

        int real = 0;

        // Compare the data and work out the value
        if (strcmp(sbit, MANCHESTER_ONE) == 0)
        {
            real = 1;
        } else if (strcmp(sbit, MANCHESTER_ZERO) == 0) {
            real = 0;
        }

        #ifdef DEBUG
            std::cout << "[decode] bit: " << bit << nbit << " [" << real << "]" << std::endl;
        #endif

        output[i] = real;
    }

    return output;
}

Manchester.h

#ifndef MANCHESTER_H
#define MANCHESTER_H

#define DEBUG

#define MANCHESTER_ONE "01"
#define MANCHESTER_ZERO "10"

class Manchester
{
public:
    static int *encode(int *data, int length);
    static int *decode(int *data, int length);
};

#endif

Answer 1

Manipulating bits

One of the areas where your code may be improved is the way it handles and compares bits. You are using char* strings to represent bits while you could have used unsigned values and bitwise operations to speed up most of your operations. Actually, you should even replace every int representing bits by unsigned since the representation of unsigned values is known and they are the tool to use when representing bits.

• You can replace the MANCHESTER_ONE and MANCHESTER_ZERO definitions by:

  #define MANCHESTER_ONE 1u
  #define MANCHESTER_ZERO 2u
  

  1u is 0b01 in binary and 2u is 0b10.

• Then you can change the following bit of code in encode:

  switch (real)
  {
  case 0:
      bit = MANCHESTER_ZERO[0] - '0'; // Subtract 48 to work out the real value
      nbit = MANCHESTER_ZERO[1] - '0';
      break;
  case 1:
      bit = MANCHESTER_ONE[0] - '0'; // Subtract 48 to work out the real value
      nbit = MANCHESTER_ONE[1] - '0';
      break;
  }
  

  By the way, the switch is useless since your are only checking for two values. You can replace it by a regular if:

  if (real == 0)
  {
      bit = (MANCHESTER_ZERO >> 1) & 1;
      nbit = MANCHESTER_ZERO & 1;
  }
  else // real == 1
  {
      bit = (MANCHESTER_ONE >> 1) & 1;
      nbit = MANCHESTER_ONE & 1;
  }
  

  Since we replaced the strings MANCHESTER_* by unsigned values, we use bitwise ANDs instead of array subtracts.

• A gain in efficiency may be obtained by refactoring this piece of code:

  // Put the data into a stringstream for comparison
  std::stringstream con;
  con << bit << nbit;
  const char* sbit = con.str().c_str();
  
  int real = 0;
  
  // Compare the data and work out the value
  if (strcmp(sbit, MANCHESTER_ONE) == 0)
  {
      real = 1;
  } else if (strcmp(sbit, MANCHESTER_ZERO) == 0) {
      real = 0;
  }
  

  std::stringstream is known to be slow since you have to convert your integer values into strings first before performing operations on them. In a for loop, it may be a big performance loss. Here is how you can refactor this bit of code:

  // Put the bits in an unsigned
  unsigned sbit = (bit << 1) | nbit;
  
  // Check only for MANCHESTER_ONE,
  // assuming that it will be equal
  // to MANCHESTER_ZERO if not
  int real = int(sbit == MANCHESTER_ONE);
  

  As said in the comments, I make the assumption that once computed, sbit can only be equal to MANCHESTER_ONE or MANCHESTER_ZERO and nothing else.

General advice about C++

I cannot see any hint that you are using C++11, therefore I will review your code as C++03 code:

• As said in the comments, allocating memory in a function and not deallocating it is not a good idea. You should simply use a std::vector to represent your arrays of numbers.
• #define is to be avoided whenever possible in C++. Consider using const variables instead.
• The standard way to check whether you are debugging is #ifndef NDEBUG (the double negative is kind of ugly, but it's the standard way).
• In C++, it is useless to write return 0; at the end of main. If no return statement is given, the compiler will add an implicit return 0;.
• If the MANCHESTER_* constants are not used anywhere else, you can define them as private static const variables of the class Machester.

---

Here is your code once refactored:

#include <iostream>
#include <stdexcept>
#include <vector>

class Manchester
{
public:

    static std::vector<unsigned> encode(const std::vector<unsigned>& data, unsigned length)
    {
        std::vector<unsigned> output(length * 2);

        for (unsigned i = 0; i < length; i++)
        {

            // Work out the array indexes to use
            unsigned bid = i * 2;
            unsigned nbid = bid + 1;

            // Get the data
            unsigned real = data[i];

            unsigned bit = 0;
            unsigned nbit = 0;

            // Work out what it is
            if (real == 0)
            {
                bit = (ZERO >> 1) & 1;
                nbit = ZERO & 1;
            }
            else // real == 1
            {
                bit = (ONE >> 1) & 1;
                nbit = ONE & 1;
            }

            #ifndef NDEBUG
                std::cout << "[encode] " << real << " [" << bit << nbit << "]" << std::endl;
            #endif

            output[bid] = bit;
            output[nbid] = nbit;
        }

        return output;
    }

    static std::vector<unsigned> decode(const std::vector<unsigned>& data, unsigned length)
    {
        if ((length % 2) != 0)
        {
            throw std::range_error("length is not a multiple of 2");
        }

        std::vector<unsigned> output(length / 2);

        for (unsigned i = 0; i < (length / 2); i++)
        {
            // Work out the array indexes to use
            unsigned bid = i * 2;
            unsigned nbid = bid + 1;

            // Get the data
            unsigned bit = data[bid];
            unsigned nbit = data[nbid];

            // Put the data into an unsigned int
            unsigned sbit = (bit << 1) | nbit;

            // Check only for MANCHESTER_ONE,
            // assuming that it will be equal
            // to MANCHESTER_ZERO if not
            unsigned real = unsigned(sbit == ONE);

            #ifndef NDEBUG
                std::cout << "[decode] bit: " << bit << nbit << " [" << real << "]" << std::endl;
            #endif

            output[i] = real;   
        }
        return output;
    }

private:

    static const unsigned ONE;
    static const unsigned ZERO;
};

const unsigned Manchester::ONE = 1u;
const unsigned Manchester::ZERO = 2u;


int main()
{
    typedef std::vector<unsigned> vec_t;

     // Some unencoded data
    unsigned data[] = { 1u, 1u, 0u, 0u };
    // Initialize vector with array
    vec_t vec(data, data+sizeof(data) / sizeof(unsigned));

    vec_t encoded = Manchester::encode(vec, 4);
    vec_t decoded = Manchester::decode(encoded, 8);
}

You can find a live working version here at Coliru.