HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

C++11 implementation of Huffman-encoding

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationhuffmanencoding

Problem

This is a c++11 implementation of Huffman-encoding that I wrote as a hobby. My main goal in writing it was to get more accustomed to c++11 and STL in general, as well as stuff like bit-manipulation.

Here are the classes bifstream and bofstream (binary ifstream/ofstream). I wrote these so that I could read in/write out a single bit at a time to a file.

bifstream.h

#pragma once

#include 
#include 

class bifstream
{
public:
    bifstream();
    ~bifstream();

public:
    void open(const char *filePath);
    void close();
    bool good();
    bool eof();
    void clear();
    bool fail();

    bool get();
    std::vector get(int bytes);
    unsigned char getByte();

private:
    std::ifstream file;
    unsigned char lastByteRead;
    short bitPosition;
};


bifstream.cpp

#include "bifstream.h"
#include 

bifstream::bifstream()
{
    lastByteRead = 0;
    bitPosition = -1;
}

bifstream::~bifstream()
{
    this->close();
}

void bifstream::open(const char *filePath)
{
    file.open(filePath, std::ios_base::in | std::ios_base::binary);
}

void bifstream::close()
{
    file.close();
}

bool bifstream::good()
{
    return file.good();
}

bool bifstream::eof()
{
    return file.eof();
}

void bifstream::clear()
{
    file.clear();
}

bool bifstream::fail()
{
    return file.fail();
}

bool bifstream::get()
{
    if (bitPosition > bitPosition;
    bitPosition--;
    return (shiftedByte & 0x01) == 1;
}

std::vector bifstream::get(int bytes)
{
    std::vector bitset;
    for (int i = 0; i > i);
        }
    }

    return c;
}


bofstream.h

#pragma once

#include 
#include 

class bofstream
{
public:
    bofstream();
    ~bofstream();

public:
    void open(const char *filePath);
    void close();
    bool good();
    bool fail();
    bool eof();
    void clear();

    void put(bool);
    void put(std::vector);

private:
    void flushBufferToFile();

private:
    std::ofstream file;
    unsigned char buffer;
    short bitPosition;
};


bofstream.

Solution

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. Take 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) is taking the vector parameter by value, thus making its own copy of it 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 & 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.

Code Snippets

void put(const std::vector<bool> & v) { ... }
         ^^^^^                   ^
for (auto i = bitset.begin(); i != bitset.end(); i++)
for (auto b : bitset) {
    // use b
}
CharCountNode::CharCountNode(Byte b, int i) : byte(b.getChar(), b.getIsTerminator())
{
    count = i;
    left = nullptr;
    right = nullptr;
 }

Context

StackExchange Code Review Q#114496, answer score: 6

Revisions (0)

No revisions yet.