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

• mr5: You shouldn't return and use a local dynamic variable. You must free it up before going out of scope, see this post about memory leak. Also, I don't see the logic by making the length of decode twice in encode since you will divide it to half w/c is equal to length of array.
• Jack Wilsdon: Ah yes, I just spotted the useless dividing and multiplying inside encode, thanks ^_^ I'll take a look at the memory leak post too :)

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.