6
\$\begingroup\$

I want to read a vector of eight bytes, convert them into hexadecimal, store them in a std::string and finally write them into a binary file.

I am looking for suggestions and recommendations to improve the efficiency and readability of the code.

#include <vector>
#include <string_view>
#include <iostream>
#include <fstream>
#include <cassert>

std::string hexify(std::vector<unsigned char> const & v) {

std::string str(2 * v.size(), 'x');
auto k = str.begin();

for(auto i{v.begin()}; i < v.end(); ++i) {
    *k++ = "0123456789ABCDEF"[*i >> 4];
    *k++ = "0123456789ABCDEF"[*i & 0x0F];
}
return str;

}

void writeFile(std::string_view str, std::string strToRead) {

assert(str.data() != nullptr);
std::ofstream f(str.data(), std::ios::binary);

if(f.is_open()) {
    f << strToRead;
}
f.close();
}

int main() {

std::vector<unsigned char> const v{'5', '1', '5', '7', '9'};
writeFile("text.bin", hexify(v));
}
\$\endgroup\$
3
  • \$\begingroup\$ And what's your question? Does the code work as intended? \$\endgroup\$ May 10 '20 at 22:43
  • \$\begingroup\$ Yes the code is working, but i need suggestions if i can improve anything here for efficiency. \$\endgroup\$
    – Owl
    May 10 '20 at 22:47
  • 1
    \$\begingroup\$ Please do not change the code in the question after an answer has been posted. As mentioned in this site guideline it violates the question answer format of Code Review. Everyone who sees the answers needs to be able to understand what the reviewer saw. \$\endgroup\$
    – pacmaninbw
    May 1 '21 at 12:14
7
\$\begingroup\$

First off, your formatting is not very good. It makes it difficult to read. I would suggest looking at your IDE's menu options and use the format command.

It seems like you're trying to reinvent the wheel here. The <ios> header contains that option already. To write the vector to a file as base 16 numbers, you can do that directly in the writeFile function:

void writeFile(std::string_view str, std::vector<unsigned char> const & v)
{

    assert(str.data() != nullptr);
    std::ofstream f(str.data(), std::ios::binary);
    for(auto c : v)
    {
        if(f.is_open())
        {
            f << std::hex << (int)c;
        }
    }
    f.close();
}

If creating the string is more to the point than writing to the file, you can use a stringstream and do the same thing:

#include <sstream>

std::string hexify(std::vector<unsigned char> const & v)
{
    std::stringstream ss;
    for(auto c : v)
    {
        ss << std::hex << (int)c;
    }
    return ss.str();
}

If characters with single digit character codes are to be used and the leading 0 is required, it's simply a matter of adding a couple of functions from the <iomanip> header:

<< std::setw(2) << std::setfill('0') << std::hex << (int)c

If reinventing the wheel is the point, first there's a tag for that.

When you have a need for literal (magic) use a constant variable. It gives meaning to anonymous values.

Generally speaking when you're using iterators in a loop, since end() is past the last element, it's better to use not equals(!=) instead of less than.

\$\endgroup\$
9
  • \$\begingroup\$ What about leading 0, does std::hex handle these? \$\endgroup\$ May 11 '20 at 2:30
  • \$\begingroup\$ @RolandIllig - added code for that. \$\endgroup\$
    – user33306
    May 11 '20 at 2:56
  • 1
    \$\begingroup\$ Instead of const std::string hexDigits = "0123456789ABCDEF";, use constexpr std::string_view. \$\endgroup\$
    – L. F.
    May 11 '20 at 11:14
  • \$\begingroup\$ “it's much more efficient” — extremely unlikely. In fact, creating a std::string instance is marginally less efficient, especially if it’s a non-static local variable. If you use the code suggested by L.F., the performance will be on par with using inline literals, but still not “more” efficient, let alone “much more”. \$\endgroup\$ May 11 '20 at 13:00
  • \$\begingroup\$ @L.F. - I modified my answer \$\endgroup\$
    – user33306
    May 11 '20 at 14:47
6
\$\begingroup\$

Disclaimer: if you want to use std::ostream and its manipulators, see @tinstaafl's answer.

Principle of least capabilities

Your functions arguments are over-constrained, compared to what is actually necessary:

  • hexify does not need to take a vector, any sequence of unsigned char would work.
  • writeFile does not need to take a copy of a string.

Careful about that string_view

Unfortunately, std::ofstream interface is antique: it still expects a C-String as an argument.

This does not mesh well with string_view, because string_view does not guarantee NUL-termination.

Thus the first argument to writeFile should be either char const* or std::string const&. I would advise the former as per the principle of least capabilities.

No Magic Constant

You use the literal "0123456789ABCDEF" twice:

  • That's one two many.
  • It's better to give a name to constants.

Range-for loops are awesome

There is place for iterator-based loops or index-based loops: when you do something tricky.

When you don't do anything special, however, use the range-for loop form: for (auto x : range) { ... }.

It immediately announces loud and clear that you're not doing anything tricky -- freeing brain cells for the reader -- and it guarantees that the loop is done as efficiently as possible -- not calling v.end() at every iteration, notably.

Implicit is more lightweight

There are perfectly good reasons to check that a file is open, or not, it allows reacting differently.

Similarly, closing a file manually rather than relying on the destructor to close allows checking for errors.

If you don't do anything special if it's not open, and you don't check whether close succeeded, then there's little point in doing either explicitly: it just clutters the code.


With all the above in mind.

//  - Principle of least capabilities, short of going template.
//  - No Magic Constant.
//  - Range-for loop.
std::string hexify(gsl::span<unsigned char> v) {
    static constexpr char HEXITS[] = "0123456789ABCDEF";

    std::string str(2 * v.size(), '\0');
    auto k = str.begin();

    for(auto c : v) {
        *k++ = HEXITS[c >> 4];
        *k++ = HEXITS[c & 0x0F];
    }

    return str;
}

//  - Principle of least capabilities.
//  - Careful about string_view.
//  - Implicit is more lightweight.
void writeFile(char const* filename, std::string const& content) {
    assert(filename != nullptr);
    std::ofstream f(filename, std::ios::binary);

    f << content;
}

//  Looking good!
int main() {
    std::vector<unsigned char> const v{'5', '1', '5', '7', '9'};
    writeFile("text.bin", hexify(v));
}

There are further potential improvements, notably around memory allocations.

Your code first allocates a string, then writes that string in the buffer of an ofstream: why not cut the middleman?

Instead you could have hexify take a std::ostream& as argument, and stream into it directly: hexify would still know nothing of writing to a file, so would be equally easy to use outside that context, and to test, just without the extraneous memory allocation.

\$\endgroup\$
4
\$\begingroup\$

A somewhat more advanced alternative could be to use iostream operators. In your example you are writing the hex data to a string, and then writing the string to a file. You could argue that this is inefficient, and the hex data could be directly written to the file in the first place without using the string for temporary storage.

Here is an example of one way you could do this. Note however that although it is marginally more efficient, it is also more verbose and more difficult to read. This is a good lesson that sometimes code that is more efficient to run is not necessarily more efficient for a human to read and maintain.

When optimising your code you need to strike a balance between the fastest leanest code possible, and something that other people can easily understand. Otherwise what is the point of saving a few seconds of execution time over the life of your program when it takes other less experienced people in your team hours to figure out what the code does?

#include <cstdint>  // uint8_t etc.
#include <fstream>
#include <iomanip>
#include <iostream>
#include <vector>

// We'll make a typedef so we don't have to keep typing the whole vector thing
// everywhere, and we'll use the newer uint8_t type rather than the less obvious
// 'unsigned char'.
typedef std::vector<uint8_t> buffer;

// We need to make a custom type so we can control which function the compiler will call.
struct hexbuffer {
    // Taking a const reference is risky, but we know in this case the lifetime of the
    // hexbuffer will match the lifetime of innerbuf, so we will be safe.  This is
    // another potentially dangerous optimisation that requires careful understanding
    // of variable scope and lifetime.  If we get it wrong the program could crash,
    // and worst case someone could exploit the crash and use it to hack into the
    // computer running this program.
    const buffer& innerbuf;
};

// This is our iostream function.  All it does is wrap the parameter up in the custom
// type so that the intended operator function below is the one that gets called.
hexbuffer hexify(const buffer& b)
{
    return { b };
}

// This operator overload is what does all the work.  Overloads require a unique function
// signature, which we achieve here by having one of the parameters be our custom type.
std::ostream& operator << (std::ostream& s, const hexbuffer& h)
{
    // Set up the stream first as it only has to be done once.
    s << std::setw(2) << std::setfill('0') << std::hex;

    // Write the hex data.
    for (auto c : h.innerbuf) {
        s << (int)c << ' ';
    }

    // iostream operators require us to return the same value we were passed.  This
    // allows them to << be << chained << together.  In other languages you see this
    // same pattern as().chained().functions().
    return s;
}

// Now all the hard stuff is done, actually using it is pretty easy.
int main()
{
    buffer example{'5', '1', '5', '7', '9'};

    // We can use our function to write to the console.
    std::cout << hexify(example) << std::endl;

    // Or we can use it to write to a file.
    std::ofstream f("text.bin");
    f << hexify(example);
    f.close();

    return 0;
}
\$\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.