5
\$\begingroup\$

I want to write a simple binary file reader and writer class. The class below can read or write file. main.cpp read 500 bytes from tmp.bin and copy at the end of the tmp_copy.bin file. How can I improve my code?

binaryfile.h

#ifndef BINARYFILE_H
#define BINARYFILE_H
#include <fstream>
#include <string>

using uchar = unsigned char;
enum FILE_TYPE {READ, WRITE, NONE};

class BinaryFile
{
public:
   BinaryFile();
   bool open(const std::string& file_path, FILE_TYPE file_type);
   bool read(uchar* buffer, size_t num_of_bytes);
   bool write(uchar* buffer, size_t num_of_bytes);
   size_t get_file_size() const;
   bool close();

private:
   std::fstream _fstream;
   std::string _file_path;
   FILE_TYPE _file_type;
   size_t _file_size;
   size_t _file_read_pos;
};

#endif // BINARYFILE_H

binaryfile.cpp

#include "binaryfile.h"
#include <filesystem>
#include <iostream>

BinaryFile::BinaryFile() :
   _file_type{NONE},
   _file_size{0},
   _file_read_pos{0}
{

}

bool BinaryFile::open(const std::string& file_path, FILE_TYPE file_type)
{
   if(_fstream.is_open())
   {
       std::cout << "You have to close the opened file\n";
       return false;
   }

   if(file_type == READ)
   {
       _file_type = READ;
       _fstream.open(file_path, std::ios::binary | std::ios::in);

   }
   else if(file_type == WRITE)
   {
       _file_type = WRITE;
       _fstream.open(file_path, std::ios::binary | std::ios::out | std::ios::app);
   }

   if(!_fstream)
   {
       std::cout << "Error in file open\n";
       _file_type = NONE;
       return false;
   }

   if(file_type == READ)
   {
       const std::filesystem::path path(file_path);
       _file_size = std::filesystem::file_size(path);
   }

   _file_path = file_path;

   return true;
}

bool BinaryFile::read(uchar *buffer, size_t num_of_bytes)
{
   if(_file_read_pos + num_of_bytes > _file_size)
   {
       std::cout << "You are trying to read more byte than file has\n";
       return false;
   }

   if(_fstream.is_open() && _file_type == READ)
   {
       _fstream.read((char*)buffer, num_of_bytes);
       _file_read_pos += num_of_bytes;
       return true;
   }

   return false;
}

bool BinaryFile::write(uchar *buffer, size_t num_of_bytes)
{
   if(_fstream.is_open() && _file_type == WRITE)
   {
       _fstream.write((char*)buffer, num_of_bytes);
       return true;
   }

   return false;
}

size_t BinaryFile::get_file_size() const
{
   return _file_size;
}

bool BinaryFile::close()
{
   if(!_fstream.is_open())
   {
       std::cout << "File is not opened\n";
       return false;
   }

   _fstream.close();
   _fstream.clear();
   _file_path.clear();
   _file_read_pos = 0;
   _file_size = 0;
   _file_type = NONE;

   return true;
}

main.cpp

#include <memory>
#include "binaryfile.h"

constexpr size_t BUFFER_SIZE{1000};

int main()
{
    BinaryFile tmp_file;
    const std::unique_ptr<uchar[]> buffer(new uchar[BUFFER_SIZE]);

    if(tmp_file.open("tmp.bin", FILE_TYPE::READ))
    {
        tmp_file.read(buffer.get(), 500);
        tmp_file.close();
    }

    if(tmp_file.open("tmp_copy.bin", FILE_TYPE::WRITE))
    {
        tmp_file.write(buffer.get(), 500);
        tmp_file.close();
    }

    return 0;
}
\$\endgroup\$
1
  • \$\begingroup\$ I'd recommend avoiding using statements in header files to alias stuff. It may lead to collisions when the header file is included in other projects. Also explicitly writing unsigned char in the public interface may increase readability, since it's a well-known data type while the custom alias uchar may be something entirely different. \$\endgroup\$ Commented Dec 4, 2023 at 11:49

2 Answers 2

6
\$\begingroup\$

Here are some things that may help you improve your code.

Avoid redundant code

The BinaryFile class adds very little other than some error messages. I'd suggest instead that the code is much shorter and simpler without it:

#include <fstream>

int main()
{
    constexpr std::size_t buffsize{500};
    char buffer[buffsize];
    std::ifstream in{"tmp.bin", std::ios::binary};
    in.read(buffer, buffsize);
    if (in)  {
        std::ofstream out{"alt.bin", std::ios::app};
        out.write(buffer, buffsize);
    }
}

Code you don't write, you don't need to debug or maintain. This kind of simplification is very useful in practice. See also ES.3 for details.

Don't use ALL_CAPS names

Your FILE_TYPE enum and the BUFFER_SIZE variable are not macros and shouldn't be macros. Don't use such names in C++. See ES.9 for more info.

Don't define variables with a leading underscore

In the global namespace, identifiers with a leading underscore are reserved names. Your class variables are not in the global namespace, but why confuse your reader? A common idiom is to use a trailing underscore, which has no such potential conflict, or to simply name things rationally without underscores -- this is the approach I take for my own code. It tends to make this easier to read in my view.

Prefer in-class initializers to constructor

There's no need to define the current default constructor for BinaryFile. There are two issues. First, once a BinaryFile is constructed this way, the only rational thing one can do with it is to call open. I'd suggest instead, that a better approach would be to define a constructor that takes a file name and mode so things can be done in a single step. Second, the constructor can be omitted entirely in favor of default data member initializers. See C.45

Fix the bug

The BinaryFile::read() function contains an error. If the enclosed std::fstream.read() function encounters eof or some other error, setting _file_read_pos += num_of_bytes is simply incorrect. Your code should check the state of the stream after the attempted read and increment by gcount() instead. Similarly, the BinaryFile::write() function fails to check the actual status of the stream after the attempted write.

\$\endgroup\$
2
\$\begingroup\$

Missing error checking

There are many things that can go wrong when operating on files. You only check if the file was opened correctly, but things can also fail when reading, writing and even closing the file. If you ignore these errors, the program will continue pretending everything is fine, but doing unexpected things or leaving behind corrupted files.

You chose to return a bool from your functions. But this is easily ignored by the caller. In fact, your example code doesn't check the return value of read(), write() and close(). Consider marking the return value [[nodiscard]] to let the compiler generated warnings if the caller forgets it.

Alternatively, consider throwing exceptions whenever an error occurs.

Unnecessary checking

On the other hand, you also added completely unnecessary checks in your code. Why get the size of a file and check against reading past the end of it, when _fstream.read(…) will already check for this?

There is also the problem that some files might not have a definite size, in which case std::filesystem::file_size() will throw an exception, preventing your code from reading those files.

How to handle errors

If you encounter an error, then write the error message to std::cerr instead of std::cout. This ensures the error message is not lost of someone redirects std::cout.

Don't continue after an error if the rest of your code wouldn't make sense any more. For example, why still try to write to tmp_copy.bin if opening tmp.bin failed?

Once you encounter an error that prevents your code from continuing normally, make sure you exit with EXIT_FAILURE. This is especially useful if you call your program from within a script, as you want the script to know that it failed as well.

Use std::vector for the buffer

While you use std::unique_ptr, which at least ensures the memory you allocate will be freed automatically, the functions you call and pass the pointer won't know the actual size of the allocated memory. Sure you also pass a num_of_bytes, but it's easy to make a mistake and pass the wrong value here.

Use a std::vector instead. Even better, make it so read() returns a vector, instead of passing a pointer to it as an argument. Consider being able to write:

auto buffer = tmp_file.read(500);
…
tmp_file.write(buffer);

Make it very clear that you append

If you open the file for FILE_TYPE::WRITE, it actually appends. This might be surprising for some people. Just make it absolutely clear, and rename WRITE to APPEND.

Simplify your code

Edward already mentioned just using std::ifstream and std::ofstream directly. It already does everything you need, and I don't think your class simplifies anything for the caller.

Also, if the goal is to just make a copy of a file (as that's what your example main() is doing), just use std::filesystem::copy().

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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