3
\$\begingroup\$

I have this function I've written to encode strings of binary data as base64 strings. I'd like to see if there are areas where it could be improved, specifically:

  • Is the function correct? Are there edge cases I haven't accounted for? (Handling arbitrary UTF-8 correctly is a "nice-to-have" but not a "need-to-have" if it significantly increases complexity, but it should be able to handle arbitrary ASCII, treating control and whitespace characters as normal characters)
  • Can the performance of the algorithm I used be improved? Am I mistaken that its time and space complexities are both O(n), and can either be improved further? Can this problem be parallelized nicely?
  • Can it safely accept arbitrary/malicious input?
  • Am I using the right language features for the best performance? (The strings in this case would be generated at runtime, but it would be interesting if anyone wanted to show how to do it with template magic at compile-time, which is a few steps above my level).
  • Is it idiomatic, modern code that follows best practices? Am I relying on any nonstandard behavior? (Not necessarily a problem, but good to know).
#include "ToBase64.h" // string, experimental/string_view
#include <cinttypes>
#include <stdexcept>

using std::length_error; // stdexcept
using std::string; // string
using std::experimental::string_view; // experimental/string_view
using std::uint8_t; // cinttypes

string to_base64(const string_view input)
{
        static constexpr auto BASE64{
                "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"
        };

        const auto len{ input.length() };
        string output;
        if (len > output.max_size() / 4 * 3 - 4) {
                throw length_error{ "Output too large to fit in std::string" };
        }
        output.reserve(len / 3 * 4 + 4);

        auto remainder{ 0 };
        uint8_t leftover{ 0 };

        for (uint8_t c : input) {
                switch ((remainder %= 3)++) {
                case 0:
                        output += BASE64[c >> 2];
                        leftover = c << 6;
                        break;
                case 1:
                        output += BASE64[(leftover >> 2) + (c >> 4)];
                        leftover = c << 4;
                        break;
                case 2:
                        output += BASE64[(leftover >> 2) + (c >> 6)];
                        output += BASE64[c & 0x3F];
                        break;
                }
        }

        switch (remainder) {
        case 1:
                output += BASE64[leftover >> 2];
                output += "==";
                break;
        case 2:
                output += BASE64[leftover >> 2];
                output += '=';
                break;
        }

        return output;
}
\$\endgroup\$
3
\$\begingroup\$
#include "ToBase64.h" // string, experimental/string_view

Best practice in C and C++ is to "#include what you use" in every file. Admittedly, in this case there is no possible way someone could refactor "ToBase64.h" to omit one of those headers, because both of those types are used in the signature of the single function that is the raison d'etre of the header. But still — I'd list #include <string> and #include <std/experimental/string_view> in "ToBase64.cc" as well as in "ToBase64.h".


using std::length_error; // stdexcept
using std::string; // string
using std::experimental::string_view; // experimental/string_view
using std::uint8_t; // cinttypes

In general, using declarations are frowned upon; although again in this case it's harmless. using declarations in header files are the really bad thing. In "ToBase64.cc", you could just as well get away with

using namespace std;

so I'm not sure why you're spending four lines to say basically the same thing. (And the comments aren't adding anything. Kill them.)


string to_base64(const string_view input)

I'm not sure what the best practice is with string_view: take it by value or by const reference? I'll assume by value is fine, since the whole point of string_view is that it's cheap to copy. But in general, taking things by const value is weird. It doesn't change the function's signature — i.e.

string to_base64(const string_view input);
string to_base64(string_view input);

redeclare the same function — so generally we'd like to omit the redundant const.

Man, so far I've commented on every single line of the code "well, this line is weird, but I guess it's harmless in this context..." ;p Don't worry, the line after this one looks great!


static constexpr smells redundant; is it actually any different from constexpr?


The expression len > output.max_size() / 4 * 3 - 4 has too many operators in too unfamiliar an order for my puny brain. I guess you're saying "suppose I base64-decoded a 0xffffffff-character string... how many characters would the decoding have? If the given plaintext has more characters than that, then let's report that the encoding failed. Except minus four for some reason."

In practice, std::string::max_size() will be on the order of 2^63 or 2^64 — so astronomically large that your algebraic expression is physically incapable of ever being false. (Logically capable, yes. Physically capable, no; no computer in the world will ever have that much memory, or enough time to initialize it if it did.) So I'd say it's better to keep the code simple and omit that check. As a bonus, this lets you kill off the temporary variable len.

If your machine is actually running out of memory, the output.reserve on the next line will throw bad_alloc anyway.

Just kidding — it'll return successfully and then you'll get oomkilled at random, later.


auto remainder{ 0 };
uint8_t leftover{ 0 };

This seems fine to me, but I'm surprised that you're following neither the "Almost Always Auto zealot" pattern of

auto remainder = 0;
auto leftover = uint8_t(0);

nor the "I'm a normal person" pattern of

int remainder = 0;
uint8_t leftover = 0;

I guess your pattern is the one that allows the most use of Uniform Initialization, though.


switch ((remainder %= 3)++)

Please don't.


output += "==";
// ...
output += '=';

This is correct, but strikes me as a refactoring typo waiting to happen. In Python, Javascript, Perl, Bash, etc., string literals can be delimited with either ' or "; but in C++, "==" is a string and '==' is a multi-character character constant with implementation-defined value (15677 on all sane platforms).

output += "==\n";
// ...
output += '=\n';  // oops

Admittedly the above code generates two warnings on both GCC and Clang, and it's a hugely unlikely refactoring anyway; but still, if this were my code, I'd use "double quotes" in both cases.


Other than all these nitpicks, it looks very clean and certainly correct!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.