Problem
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
Solution
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
andMANCHESTER_ZERO
definitions by:#define MANCHESTER_ONE 1u #define MANCHESTER_ZERO 2u
1u
is0b01
in binary and2u
is0b10
. 
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 regularif
: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_*
byunsigned
values, we use bitwiseAND
s 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 afor
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 toMANCHESTER_ONE
orMANCHESTER_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 usingconst
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 ofmain
. If noreturn
statement is given, the compiler will add an implicitreturn 0;
.  If the
MANCHESTER_*
constants are not used anywhere else, you can define them asprivate
static const
variables of the classMachester
.
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.