Return to Revisions

Looks well written overall. Good job. I'll just give a few quick remarks about the code. Compression is not really my field.

• Unnecessary destructors in bofstream and bifstream. They already hold Standard streams that automatically close the underlaying file handles in their own destructors, so you're just replicating work by calling close() explicitly. Once you remove that, you'll end up with empty destructors that should be removed. Let the compiler supply a default when you have no manual cleanup to perform in your classes. Actually, bofstream might need to flushBufferToFile(), so that one might need a destructor after all...

• Mark methods that aren't changing member data with const. E.g.: eof, good, fail, etc. That a look into what const member functions mean in C++. This is an important thing to use and I see that you are not being consistent with it.

• bofstream::put(std::vector<bool>) is taking the vector parameter by value, thus making its own copy of if just to iterate the array. You don't need that copy if you're just inspecting the collection. Pass by const reference instead:

    void put(const std::vector<bool> & v) { ... }
             ^^^^^                   ^
  

In general you should pass objects by reference if you're only looking at the object without wanting to store an actual copy of it somewhere. Native types like integers are of course still passed by value, since copying those is free and implemented in the hardware.

• Use range based for loops when you're iterating a standard collection from back-to-back. Instead of this:

for (auto i = bitset.begin(); i != bitset.end(); i++)

You can do this:

    for (auto b : bitset) {
        // use b
    }

• Prefixing member variables or methods with this-> is generally not a good practice. Not only it is unnecessary and verbose, but it can also hide names that shadow each other, which can be a serious problem.

• Byte's default constructor doesn't initialize its member data. Always initialize data to some known value to make your code deterministic.

• Byte's destructor is empty, so should be removed. Same as mentioned in the fist point.

• Why didn't you initialize all members of CharCountNode in the initializer list?

CharCountNode::CharCountNode(Byte b, int i) : byte(b.getChar(), b.getIsTerminator())
{
    count = i;
    left = nullptr;
    right = nullptr;
 }

• Use std::snprintf over sprintf_s or sprintf. It takes a count parameter to prevent buffer overruns.