## File organization
You should have three files:
* `bitspeak.hpp`, the header containing only the minimally necessary definitions
* `bitspeak.cpp`, the implementation of the functions defined in the header
* `bitspeak_test.cpp`, the tests for the implementation
## bitspeak.hpp
#ifndef BITSPEAK_HPP
#define BITSPEAK_HPP
#include <string>
namespace bitspeak
{
std::string encode(const std::string &data);
std::string decode(const std::string &text);
}
#endif
This is the simplest definition of the interface that a caller needs. The `std::string` class can handle all bytes, therefore it is suitable both for strings of `char`, as well as for byte arrays.
## bitspeak.cpp
Instead of writing the `vows` and `cons` in a single line, I rather like them to look like this:
static const std::string vows[32] = {
"a", "e", "i", "o", "u", "w", "y", "ae",
"ai", "ao", "au", "aw", "ay", "ea", "ee", "ei",
"eo", "eu", "ey", "ia", "io", "iu", "oa", "oi",
"ou", "ow", "oy", "ui", "uo", "uy", "we", "wi"
};
static const std::string cons[128] = {
"b", "c", "d", "f", "g", "h", "j", "k",
"l", "m", "n", "p", "q", "r", "s", "t",
"v", "x", "z", "bc", "bl", "br", "bs", "ch",
"chs", "cht", "ck", "cl", "cr", "cs", "ct", "fl",
"fr", "fs", "ft", "gl", "gm", "gr", "gs", "gz",
"hd", "hf", "hr", "hs", "ht", "hv", "jr", "js",
"jt", "kl", "kr", "kt", "lb", "lc", "ld", "lg",
"lm", "ln", "lp", "ls", "lt", "lv", "mb", "mn",
"mp", "mr", "nd", "nk", "nm", "np", "ns", "nt",
"nv", "nz", "pl", "pr", "ps", "pt", "pz", "rb",
"rc", "rch", "rd", "rf", "rg", "rk", "rl", "rm",
"rn", "rp", "rr", "rs", "rt", "rv", "rz", "sb",
"sc", "sch", "sd", "sf", "sh", "sl", "sm", "sn",
"sp", "sq", "sr", "st", "sth", "str", "sv", "sz",
"tch", "th", "tl", "tn", "tr", "ts", "vl", "vr",
"vs", "vz", "xc", "xz", "zd", "zl", "zp", "zt"
};
Grouping them in 8 per line lets the reader quickly check that these are indeed 32 respectively 128 strings.
----
The implementation of `bitspeak::encode` can be done in a single pass without unnecessary copying:
namespace bitspeak {
std::string encode(const std::string &data)
{
std::ostringstream out;
std::size_t len = data.length();
for (std::size_t i = 0; i < len; i += 3) {
uint8_t d0 = data[i + 0];
uint8_t d1 = i + 1 < len ? data[i + 1] : 0;
uint8_t d2 = i + 2 < len ? data[i + 2] : 0;
uint8_t cc0 = (d0 & 0xFE) >> 1;
uint8_t vv0 = ((d0 & 0x01) << 4) | ((d1 & 0xF0) >> 4);
uint8_t cc1 = ((d1 & 0x0F) << 3) | ((d2 & 0xE0) >> 5);
uint8_t vv1 = d2 & 0x1F;
out << cons[cc0];
out << vows[vv0];
out << (i + 1 < len ? cons[cc1] : "?");
out << (i + 2 < len ? vows[vv1] : "?");
}
return out.str();
}
} // end of namespace
I have changed the following things:
* There is a single loop, which processes 3 bytes at a time (note that `i += 3` is used here instead of the usual `i++`).
* Instead of using `uint32_t`, the code uses `std::size_t`, which is exactly the type returned by `std::string.length()`.
* The first block that defines `d0`, `d1` and `d2` makes sure that the data bytes are interpreted as unsigned.
* They are then converted in the same way that you used in the `ca3_to_ca4` function.
* The last block appends the consonants and vowels. Even in the last group, the first pair of consonants and vowels is always relevant, since the bits from `d0` influence `cc0` and `vv0`. The remaining characters may need to be replaced with the padding character.
* The class `std::ostringstream` is suitable for efficient appending to a string.
----
For decoding a string of consonants and vowels into the original data, idiomatic C++ code does not copy much data around. Instead, it uses indexes or iterators into the strings, which are only pointers that can be efficiently passed around.
The basic idea is this:
* find a sequence of consonants
* find a sequence of vowels
* find a sequence of consonants
* find a sequence of vowels
* map the above sequences to indices
* bitshift the indices around to produce the original data
* repeat until the end of the encoded string
The padding needs to be handled specially since it is only allowed at the very end. And this is the code I came up with:
std::string decode(const std::string &text)
{
std::size_t i = 0;
std::size_t len = text.length();
std::ostringstream out;
while (i < len) {
auto cc0start = i;
while (i < len && con_l.find(text[i]) != std::string::npos)
i++;
auto vv0start = i;
while (i < len && vow_l.find(text[i]) != std::string::npos)
i++;
auto cc1start = i;
while (i < len && con_l.find(text[i]) != std::string::npos)
i++;
auto vv1start = i;
while (i < len && vow_l.find(text[i]) != std::string::npos)
i++;
if (i == cc0start)
throw "invalid encoded data (neither consonant nor vowel)";
auto cc0 = text.substr(cc0start, vv0start - cc0start);
auto vv0 = text.substr(vv0start, cc1start - vv0start);
auto cc1 = text.substr(cc1start, vv1start - cc1start);
auto vv1 = text.substr(vv1start, i - vv1start);
auto cons_end = cons + 128;
auto vows_end = vows + 32;
auto cc0bits = std::find(cons, cons_end, cc0) - cons;
auto vv0bits = std::find(vows, vows_end, vv0) - vows;
auto cc1bits = std::find(cons, cons_end, cc1) - cons;
auto vv1bits = std::find(vows, vows_end, vv1) - vows;
if (cc0bits == 128 || vv0bits == 32)
throw "invalid encoded data (unknown letter combination)";
unsigned char d0 = (cc0bits << 1) | ((vv0bits & 0x10) >> 4);
out << d0;
if (cc1bits == 128) {
if (text.substr(i) != "??")
throw "invalid encoded data (padding)";
break;
}
unsigned char d1 = ((vv0bits & 0x0F) << 4) | ((cc1bits & 0x78) >> 3);
out << d1;
if (vv1bits == 32) {
if (text.substr(i) != "?")
throw "invalid encoded data (padding)";
break;
}
unsigned char d2 = ((cc1bits & 0x07) << 5) | (vv1bits & 0x1F);
out << d2;
}
return out.str();
}
I know that the above code violates some best practice rules:
* it contains duplicate code in the `while` loops. I did this since extracting the loops to a separate function would add even more code.
* it contains hard-coded numbers, which some readers may consider _magic numbers_. But since they are powers of 2 and mentioned in the specification, they are not magic, they are only numbers, like the other numbers in the bit-shifting code.
* these numbers are even repeated in several places. I did not make them constants since they will not change anyway. And even if they should do, each `128` in the code has exactly the same meaning, so they can all be changed doing a simple text replacement.
----
Your `is_vowel` function can be shortened a bit.
bool is_vowel(char x) { bool o = false; std::size_t i; for (i = 0; i < vow_l.size(); i++) { if (x == vow_l[i]) { o = true; break; } } return o; }
bool is_vowel(char x) { std::size_t i; for (i = 0; i < vow_l.size(); i++) { if (x == vow_l[i]) { return true; } } return false; }
bool is_vowel(char x) { for (std::size_t i = 0; i < vow_l.size(); i++) { if (x == vow_l[i]) { return true; } } return false; }
bool is_vowel(char x) { return vow_l.find(x) != std::string::npos; }
Instead of using `break` and a return value, it is often simpler to just `return value`, e.g. in `string_find`.