Get histogram of bytes in any set of files in C++14 - take II

Question

Intro

(This post is the continuation of Get histogram of bytes in any set of files in C++14.)

This time I have added some ASCII art for visualizing the histogram. Also, if the program is invoked without command line arguments, it will expect the data from std::cin.

Code

#include <algorithm>
#include <array>
#include <cstdlib>
#include <fstream>
#include <iomanip>
#include <ios>
#include <istream>
#include <iostream>
#include <sstream>
#include <string>
#include <vector>

using std::array;
using std::cin;
using std::cout;
using std::ifstream;
using std::istream;
using std::string;
using std::stringstream;
using std::vector;

static constexpr size_t TERMINAL_WIDTH = 80;

class ByteHistogram {
    private:

        static constexpr size_t HISTOGRAM_WIDTH_ = 0x100;
        typedef array<size_t, HISTOGRAM_WIDTH_> Histogram;
        Histogram histogram_;

        friend std::ostream& operator<<(std::ostream& os, 
                                        const ByteHistogram& hist);

    public:
        ByteHistogram() {
            std::fill(histogram_.begin(), 
                      histogram_.end(),
                      0);
        }

        void count(unsigned char ch) {
            histogram_[ch]++;
        }

        Histogram::const_iterator cbegin() const {
            return histogram_.cbegin();
        }

        Histogram::const_iterator cend() const {
            return histogram_.cend();
        }

} byteHistogram; // Instantiate a global object.

static size_t computeMaximumCount(const ByteHistogram& histogram) {
    size_t maximumCount = 0;

    for (auto it = histogram.cbegin(); it != histogram.cend(); it++) {
        maximumCount = std::max(maximumCount, *it);
    }

    return maximumCount;
}

static size_t computeCounterStringLength(const size_t maximumCount) {
    stringstream ss;
    ss << maximumCount;
    return ss.str().length();
}

static string computeBar(const size_t count,
                         const size_t maximumCount,
                         const size_t maximumCountLength) {

    const size_t availableSpace = TERMINAL_WIDTH - 7 - maximumCountLength;
    const size_t barLength = (size_t) 
        availableSpace * (((float) count)
                        / ((float) maximumCount));
    stringstream ss;

    for (size_t i = 0; i < barLength; i++) {
        ss << '*';
    }

    return ss.str();
}

std::ostream& operator<<(std::ostream& os,
                         const ByteHistogram& hist) {

    const size_t maximumCount = computeMaximumCount(byteHistogram);
    const size_t countStringLength = computeCounterStringLength(maximumCount);
    stringstream ss;
    ss << "0x%02x: %" << countStringLength << "d %s\n";
    const string formatString = ss.str();
    const char* cFormatString = formatString.c_str();

    for (size_t i = 0; i != ByteHistogram::HISTOGRAM_WIDTH_; i++) {
        const size_t count = hist.histogram_[i];
        const string bar = computeBar(count, 
                                      maximumCount,
                                      countStringLength);
        std::printf(cFormatString,
                    static_cast<unsigned char>(i), 
                    count,
                    bar.c_str());
    }

    return os;
}

static void processCin();

static void processFile(const string& fileName);

static vector<string> getFileNames(const int argc, char* argv[]);

int main(int argc, char* argv[])
{   
    if (argc == 1) {
        processCin();
        return EXIT_SUCCESS;
    }

    vector<string> fileNames = getFileNames(argc, argv);

    for (auto it = fileNames.cbegin(); it != fileNames.cend(); it++) {
        processFile(*it);
    }

    cout << byteHistogram;
    return EXIT_SUCCESS;
}

static vector<string> getFileNames(const int argc,
                                   char* argv[]) {
    vector<string> fileNames;

    for (size_t i = 1; i < argc; i++) {
        fileNames.push_back(string(argv[i]));
    }

    return fileNames;
}

static void processCin() {
    while (!cin.eof()) {
        const unsigned char ch = cin.get();

        if (cin.fail() || cin.bad()) {
            break;
        }

        byteHistogram.count(ch);
    }

    cout << byteHistogram;
}

static void throwOnFailedFile(const string& fileName) {
    std::ios_base::failure exception("Could not process file \"" + fileName + "\".");

    throw exception;
}

static void processFile(const string& fileName) {
    ifstream stream(fileName, std::ios::binary);

    while (!stream.eof()) {
        unsigned char ch;
        ch = static_cast<unsigned char>(stream.get());

        if (!stream.good() || stream.fail()) {
            break;
        }

        byteHistogram.count(ch);
    }

    stream.close();
}

Typical output

C:\Users\rodio\OneDrive\Desktop>type SkipList.java | ByteHistogram.cpp.exe
0x00:    0
0x01:    0
0x02:    0
0x03:    0
0x04:    0
0x05:    0
0x06:    0
0x07:    0
0x08:    0
0x09:    1
0x0a:  233 ****
0x0b:    0
0x0c:    0
0x0d:    0
0x0e:    0
0x0f:    0
0x10:    0
0x11:    0
0x12:    0
0x13:    0
0x14:    0
0x15:    0
0x16:    0
0x17:    0
0x18:    0
0x19:    0
0x1a:    0
0x1b:    0
0x1c:    0
0x1d:    0
0x1e:    0
0x1f:    0
0x20: 3313 *********************************************************************
0x21:    6
0x22:   14
0x23:    0
0x24:    0
0x25:    0
0x26:    0
0x27:   46
0x28:   63 *
0x29:   63 *
0x2a:    4
0x2b:   53 *
0x2c:   11
0x2d:   16
0x2e:   63 *
0x2f:    8
0x30:    8
0x31:   22
0x32:    4
0x33:    2
0x34:    3
0x35:    3
0x36:    0
0x37:    0
0x38:    0
0x39:    0
0x3a:    8
0x3b:  103 **
0x3c:   27
0x3d:   74 *
0x3e:   27
0x3f:    3
0x40:    3
0x41:    5
0x42:    4
0x43:   38
0x44:    0
0x45:    0
0x46:    0
0x47:    0
0x48:    4
0x49:   59 *
0x4a:    0
0x4b:   24
0x4c:   12
0x4d:   49 *
0x4e:   12
0x4f:    8
0x50:    3
0x51:    0
0x52:   19
0x53:   20
0x54:   13
0x55:    0
0x56:    1
0x57:    2
0x58:    0
0x59:    0
0x5a:    0
0x5b:   57 *
0x5c:   11
0x5d:   57 *
0x5e:    0
0x5f:    0
0x60:    0
0x61:  211 ****
0x62:   21
0x63:   75 *
0x64:  140 **
0x65:  296 ******
0x66:   15
0x67:   61 *
0x68:  110 **
0x69:  187 ***
0x6a:   10
0x6b:   14
0x6c:  123 **
0x6d:   16
0x6e:  242 *****
0x6f:  152 ***
0x70:   56 *
0x71:    2
0x72:  242 *****
0x73:   86 *
0x74:  323 ******
0x75:   44
0x76:   46
0x77:   43
0x78:  151 ***
0x79:   26
0x7a:    1
0x7b:   33
0x7c:    5
0x7d:   33
0x7e:    0
0x7f:    0
0x80:    0
0x81:    0
0x82:    0
0x83:    0
0x84:    0
0x85:    0
0x86:    0
0x87:    0
0x88:    0
0x89:    0
0x8a:    0
0x8b:    0
0x8c:    0
0x8d:    0
0x8e:    0
0x8f:    0
0x90:    0
0x91:    0
0x92:    0
0x93:    0
0x94:    0
0x95:    0
0x96:    0
0x97:    0
0x98:    0
0x99:    0
0x9a:    0
0x9b:    0
0x9c:    0
0x9d:    0
0x9e:    0
0x9f:    0
0xa0:    0
0xa1:    0
0xa2:    0
0xa3:    0
0xa4:    0
0xa5:    0
0xa6:    0
0xa7:    0
0xa8:    0
0xa9:    0
0xaa:    0
0xab:    0
0xac:    0
0xad:    0
0xae:    0
0xaf:    0
0xb0:    0
0xb1:    0
0xb2:    0
0xb3:    0
0xb4:    0
0xb5:    0
0xb6:    0
0xb7:    0
0xb8:    0
0xb9:    0
0xba:    0
0xbb:    0
0xbc:    0
0xbd:    0
0xbe:    0
0xbf:    0
0xc0:    0
0xc1:    0
0xc2:    0
0xc3:    0
0xc4:    0
0xc5:    0
0xc6:    0
0xc7:    0
0xc8:    0
0xc9:    0
0xca:    0
0xcb:    0
0xcc:    0
0xcd:    0
0xce:    0
0xcf:    0
0xd0:    0
0xd1:    0
0xd2:    0
0xd3:    0
0xd4:    0
0xd5:    0
0xd6:    0
0xd7:    0
0xd8:    0
0xd9:    0
0xda:    0
0xdb:    0
0xdc:    0
0xdd:    0
0xde:    0
0xdf:    0
0xe0:    0
0xe1:    0
0xe2:    0
0xe3:    0
0xe4:    0
0xe5:    0
0xe6:    0
0xe7:    0
0xe8:    0
0xe9:    0
0xea:    0
0xeb:    0
0xec:    0
0xed:    0
0xee:    0
0xef:    0
0xf0:    0
0xf1:    0
0xf2:    0
0xf3:    0
0xf4:    0
0xf5:    0
0xf6:    0
0xf7:    0
0xf8:    0
0xf9:    0
0xfa:    0
0xfb:    0
0xfc:    0
0xfd:    0
0xfe:    0
0xff:    0

Critique request

Am I getting anywhere? Please tell me whatever comes to mind.

Answer 1

The code looks much improved compared to version 1. Most of what I have to say may be subjective preference rather than required actions.

I'll step through the code in sequential order, rather than trying to pick out the biggest criticisms first.

---

#include <algorithm>
#include <array>
#include <cstdlib>
#include <fstream>
#include <iomanip>
#include <ios>
#include <istream>
#include <iostream>
#include <sstream>
#include <string>
#include <vector>

It's nice that these are in sorted order - makes it easy to see that we have omitted <cstdio>, needed for std::printf.

using std::array;
using std::cin;
using std::cout;
using std::ifstream;
using std::istream;
using std::string;
using std::stringstream;
using std::vector;

I'm not really a big fan of bringing standard names into the global namespace. This is at least selective so I won't complain too strongly. A notable absence here is std::size_t which you seem to assume the library also defines in global namespace. That's not mandated, posing a potential portability risk.

static constexpr size_t TERMINAL_WIDTH = 80;

Convention is that we use all-caps names only for preprocessor macros (which don't obey C++ rules of scope, for instance).

class ByteHistogram {
    private:

We don't need to write private: here, since that's the default access level in a class. But it does no harm.

        static constexpr size_t HISTOGRAM_WIDTH_ = 0x100;
        typedef array<size_t, HISTOGRAM_WIDTH_> Histogram;
        Histogram histogram_;

Again, the all-caps name screams "CAUTION" when it's not justified. I (and many others) find using easier to read than typedef:

using Histogram = std::array<std::size_t, HISTOGRAM_WIDTH_>;

However, we might not need to name this type - see below.

The use of std::size_t for the count may be a bug. There's a possibility that content from files could cause std::size_t to overflow, since files may be much larger than the C++ object limit. It would be safer to use a size with a guaranteed capacity such as std::uint_fast64_t. Although that could theoretically overflow, I think that would take around 30 years if we consume input at 16 GB/s, which is probably good enough for our purposes.

        friend std::ostream& operator<<(std::ostream& os, 
                                        const ByteHistogram& hist);

That looks perfect. But we might not need it to be a friend, if we tweak the public interface a little.

    public:
        ByteHistogram() {
            std::fill(histogram_.begin(), 
                      histogram_.end(),
                      0);
        }

If this were C++20, we would have the Ranges version which is simpler:

          std::ranges::fill(histogram_, 0);

        void count(unsigned char ch) {
            histogram_[ch]++;
        }

count() could be read as either a verb or a noun, but I think it's clear enough that this is adding to the histogram. As an alternative, we could call it insert().

I would normally write ++histogram_[ch] when we discard the value like this: it makes no difference here with an integer type, but prefix increment can be cheaper with some types (e.g. iterators) so it's a useful habit to adopt.

        Histogram::const_iterator cbegin() const {
            return histogram_.cbegin();
        }

        Histogram::const_iterator cend() const {
            return histogram_.cend();
        }

If we want our histogram to be useful as a standard range (e.g. for range-based for), we should provide begin() and end(). They can just return cbegin() and cend() respectively (since we don't want to give out writeable iterators). It also helps some algorithms if we provide size().

} byteHistogram; // Instantiate a global object.

I don't really like this - defining the type and an instance together, and also creating a global when I think it would be better as an automatic variable in function scope.

static size_t computeMaximumCount(const ByteHistogram& histogram) {

This looks like it should be a member function:

std::size_t computeMaximumCount() const

    size_t maximumCount = 0;

    for (auto it = histogram.cbegin(); it != histogram.cend(); it++) {
        maximumCount = std::max(maximumCount, *it);
    }

Instead of writing a loop, we could use the std::max_element() from <algorithm>:

private:
    std::size_t computeMaximumCount() const
    {
        return *std::max_element(histogram_.begin(), histogram.end());
    }

As this is used in only one place, it might be better to simply inline this.

static size_t computeCounterStringLength(const size_t maximumCount) {
    stringstream ss;
    ss << maximumCount;
    return ss.str().length();
}

This is one case where the C-style I/O might be more efficient - std::snprintf() can tell us how many characters it would print:

static int computeCounterStringLength(const size_t maximumCount)
{
    return std::snprintf(nullptr, 0, "%zu", maximumCount);
}

static string computeBar(const size_t count,
                         const size_t maximumCount,
                         const size_t maximumCountLength) {

    const size_t availableSpace = TERMINAL_WIDTH - 7 - maximumCountLength;
    const size_t barLength = (size_t) 
        availableSpace * (((float) count)
                        / ((float) maximumCount));

That's a redundant cast of availableSpace - I think what was intended was to cast the result of the multiplication:

    auto const availableSpace = static_cast<double>(TERMINAL_WIDTH - 7 - maximumCountLength);
    auto const barLength = static_cast<size_t>(availableSpace * count / maximumCount);

We might consider doing it all as integer arithmetic if count < SIZE_MAX / availableSpace.

    stringstream ss;

    for (size_t i = 0; i < barLength; i++) {
        ss << '*';
    }

    return ss.str();
}

Instead of using a string stream, we can simply construct a std::string with a specified number of a specified character:

    return std::string(barLength, '*');

std::ostream& operator<<(std::ostream& os,
                         const ByteHistogram& hist) {

    const size_t maximumCount = computeMaximumCount(byteHistogram);
    const size_t countStringLength = computeCounterStringLength(maximumCount);
    stringstream ss;
    ss << "0x%02x: %" << countStringLength << "d %s\n";
    const string formatString = ss.str();
    const char* cFormatString = formatString.c_str();

    for (size_t i = 0; i != ByteHistogram::HISTOGRAM_WIDTH_; i++) {
        const size_t count = hist.histogram_[i];
        const string bar = computeBar(count, 
                                      maximumCount,
                                      countStringLength);

Instead of creating a new string object each iteration, we could create a single max-length string, and then output different length substrings of a view onto it. Or we can set the output stream's fill character to * and print an empty string to a fixed-width field:

        auto const bar_len = static_cast<size_t>(availableSpace * count / maximumCount);

        std::cout << std::setfill('*') << std::setw(bar_len) << "" << '\n';

        std::printf(cFormatString,

Using a run-time format string isn't the best, because it prevents compilers from checking that it's consistent with the actual passed arguments.

In this case, I think I'd prefer to use C++ formatted output and manipulators:

        std::cout << "0x" << std::hex << std::setfill('0') << std::setw(2) << i << ": "
                  << std::dec << std::setfill(' ') << std::setw(countStringLength) << count << ' '
                  << std::setfill('*') << std::setw(bar_len) << "" << '\n';

static void processCin();

static void processFile(const string& fileName);

static vector<string> getFileNames(const int argc, char* argv[]);

It's usually better to define these as we go, rather than forward-declaring them. Just move main() to the bottom and it all works.

int main(int argc, char* argv[])
{   
    if (argc == 1) {
        processCin();
        return EXIT_SUCCESS;
    }

    vector<string> fileNames = getFileNames(argc, argv);

Oh, we don't need to create a vector or any string objects. It's more efficient here to just pass plain (C-style) string pointers. That means no memory allocations, so minimal overhead.

    for (auto it = fileNames.cbegin(); it != fileNames.cend(); it++) {
        processFile(*it);
    }

This may be better as a range-based for:

    for (auto const& path: files) {
        processFile(path);
    }

static vector<string> getFileNames(const int argc,
                                   char* argv[]) {
    vector<string> fileNames;

    for (size_t i = 1; i < argc; i++) {

We have a mismatch here between signed int and unsigned std::size_t. That's easily avoided by changing to int i. This could be a symptom that not enough compiler warnings have been enabled in your development builds.

static void processCin() {

I think it's better to have a function that can read from any input stream. Then we could use it to read from files, too. I think I would make it a member function of the histogram class:

    std::istream& count(std::istream&is)
    {
        ⋮
        return is;
    }

    while (!cin.eof()) {
        const unsigned char ch = cin.get();

while (!eof) is a common antipattern. The correct use is to attempt the get() and then check for eof:

        std::istream::int_type c;
        while ((c = is.get()) != std::istream::traits_type::eof()) {
            count(static_cast<unsigned char>(c));
        }

static void throwOnFailedFile(const string& fileName) {

This function is never called.

static void processFile(const string& fileName) {
    ifstream stream(fileName, std::ios::binary);

    ⋮

    stream.close();
}

The close() is unnecessary, because the stream's destructor closes it for us. It's generally a good idea to check whether the stream has failed before we (explicitly or implicitly) close it so that we don't produce misleading output based on a partially-read input.

---

Modified code

With some further inlining of single-use functions and other minor tweaks:

#include <algorithm>
#include <array>
#include <cstdlib>
#include <cstdint>
#include <cstdio>
#include <climits>
#include <fstream>
#include <iomanip>
#include <iostream>
#include <string>

static constexpr std::size_t screen_width = 80;

class ByteHistogram
{
    std::array<std::uint_fast64_t, UCHAR_MAX + 1> data = {};

public:
    ByteHistogram()
    {
        std::fill(data.begin(), data.end(), 0);
    }

    void insert(unsigned char ch)
    {
        ++data[ch];
    }

    std::istream& insert(std::istream&is)
    {
        std::istream::int_type c;
        while ((c = is.get()) != std::istream::traits_type::eof()) {
            insert(static_cast<unsigned char>(c));
        }
        return is;
    }

    auto begin() const {
        return data.begin();
    }

    auto end() const {
        return data.end();
    }

    std::size_t size() const {
        return data.size();
    }

    auto const& operator[](std::size_t i) const {
        return data[i];
    }
};

std::ostream& operator<<(std::ostream& os,
                         const ByteHistogram& hist)
{
    auto const maximum_count = *std::max_element(hist.begin(), hist.end());
    auto const count_width = std::snprintf(nullptr, 0, "%zu", maximum_count);
    auto const max_bar = static_cast<double>(screen_width - sizeof "0x00: ." - count_width);

    std::size_t i = 0;
    for (auto const count: hist) {
        auto const bar_len = static_cast<int>(max_bar * static_cast<double>(count)
                                              / static_cast<double>(maximum_count));
        os << "0x" << std::hex << std::setfill('0') << std::setw(2) << i++ << ": "
           << std::dec << std::setfill(' ') << std::setw(count_width) << count << ' '
           << std::setfill('*') << std::setw(bar_len) << "" << '\n';
    }

    return os;
}

int main(int argc, char* argv[])
{
    ByteHistogram h;

    if (argc == 1) {
        h.insert(std::cin);
        if (std::cin.bad()) {
            std::cerr << "Reading from standard input failed.\n";
            return EXIT_FAILURE;
        }
    } else {
        for (int i = 1;  i < argc;  ++i) {
            std::ifstream stream{argv[i], std::ios::binary};
            h.insert(stream);
            if (stream.bad()) {
                std::cerr << "Reading from " << argv[i] << " failed.\n";
                return EXIT_FAILURE;
            }
        }
    }

    std::cout << h;
    return EXIT_SUCCESS;
}
```