Manchester encoder/decoder

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 
#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
#include
#include
#include

#ifdef DEBUG
#include
#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

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 
#include 
#include 

class Manchester
{
public:

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

        for (unsigned i = 0; i > 1) & 1;
                nbit = ZERO & 1;
            }
            else // real == 1
            {
                bit = (ONE >> 1) & 1;
                nbit = ONE & 1;
            }

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

        std::vector output(length / 2);

        for (unsigned i = 0; i  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.

StackExchange Code Review Q#33396, answer score: 7