C++ File Converter Program

Question

Looking for input on how to clean up my code. I am writing a file converter that can read different but related file types (relatable like a wav and mp3 encode audio or how a jpg and tiff encode image data) and convert them to a desired file type. I got it to a decent stopping point where I can handle one file type to output to CSV and want to take this time to refactor.

Please let me know if my program can benefit from any design principles or if I should break down objects/classes further or I'm doing anything just plain wrong. I am trying my best to improve on my own. Ultimate goal is add a GUI and a plotter function for the data to look at before converting.

Main.cpp

#include <iostream>
#include <fstream>

#include "file_interface.h"
#include "simple_parser.h"

int main(int argc, char** argv)
{
    InputParser input(argc, argv);
    const std::string& filepath = input.getCmdOption("-f");

    if (!filepath.empty())
    {
        
        max_temp_data values;
        try
        {
            FileInterface file_interface;
            values = file_interface.extract_from(filepath);
            
        }
        catch (const std::runtime_error & e) {
            std::cout << e.what();
        }

        std::string output_path = "C:\\Documents\\example.csv";
        FileInterface file_interface;
        file_interface.output_new(output_path, values);
    }

    return 0;
}

file_interface.h

#include <fstream>
#include <vector>
#include <memory>

#include "base_definition.h"

struct max_temp_data
{
    int total_points = 0;
    double start_time = 0.0;
    double end_time = 0.0;
    std::vector<float> T_values = {0};


};

//
// File interface loads the file to be converted and assigns the proper encoder/decoder
//
class FileInterface
{
public:
    FileInterface();
    ~FileInterface();

    void initialize_from(const std::string& filepath);
    max_temp_data extract_from(const std::string& filepath);
    void output_new(const std::string& extension, max_temp_data& values);

    friend std::ostream& operator<<(std::ostream& os, const max_temp_data& mhd);
private:

    void load_definition(const std::string& extension);

    //
    // Current loaded file
    std::ifstream input_file_;

    std::unique_ptr<BaseDefinition> definition_;

};

inline std::ostream& operator<<(std::ostream& os, const max_temp_data& mhd)
{
    os << mhd.total_points << '/' << mhd.start_time << '/' << mhd.end_time;
    return os;
}

file_interface.cpp

#include <fstream>
#include <iostream>
#include <string>
#include <filesystem>

#include "file_interface.h"
#include "peak_definition.h"
#include "csv_definition.h"


FileInterface::FileInterface()
{
}

FileInterface::~FileInterface()
{
    if (input_file_.is_open())
    {
        input_file_.close();
    }
}

void FileInterface::load_definition(const std::string& extension)    
{
    if (extension == std::string(".peak"))
    {
        std::cout << "PEAK Definition" << "\n";
        definition_ = std::make_unique<PeakDefinition>();
    }
    if (extension == std::string(".csv"))
    {
        std::cout << "CSV Definition" << "\n";
        definition_ = std::make_unique<CsvDefinition>();
    }
}

void FileInterface::initialize_from(const std::string& filepath)
{
    auto extension = std::filesystem::path(filepath).extension().string();

    load_definition(extension);
    if (!definition_)
    {
        //Throw invalid file type
        throw std::runtime_error(extension + ": is not a valid extension");
    }

}

max_temp_data FileInterface::extract_from(const std::string& filepath)
{
    input_file_.open(filepath, std::ios::binary);
    if (!input_file_)
    {
        //Throw file open error
        throw std::runtime_error(filepath + ": could not be opened ");
    }

    initialize_from(filepath);
    max_temp_data values = { 0 };

    // Extract Header
    definition_->extract_header(input_file_);

    if (input_file_.fail())
    {
        throw std::runtime_error("Failed to read header");
    }

    if (!definition_->validate_descriptor())
    {
        throw std::runtime_error("File bytes do not match expected bytes.");
    }

    values.total_points     = definition_->extract_total_points();
    values.start_time   = definition_->extract_start_freq();
    values.end_time = definition_->extract_end_freq();

    // Extract Data
    input_file_.seekg(0, std::ios::end);
    auto length = input_file_.tellg();
    if(length < definition_->get_start_pos())
    {
        throw std::runtime_error("Start position of file exceeds length of file: " + (int)length);
    }

    input_file_.seekg(definition_->get_start_pos(), std::ios_base::beg);
    for (int i = 0; i < values.total_points; ++i)
    {
        auto val = definition_->extract_data_point(input_file_);
        values.T_values.push_back(val);

        if (input_file_.fail())
        {
            throw std::runtime_error("Failed to read data at position: " + i);
        }
    }

    input_file_.close();
    return values;
}

void FileInterface::output_new(const std::string& output_path, max_temp_data &values)
{
    initialize_from(output_path);
    std::ofstream output_file(output_path);
    definition_->write_header(output_file, values.total_points, values.start_time, values.end_time);

    for (auto& val : values.T_values)
    {
        definition_->write_data(output_file, val);
    }
    
    output_file.close();
}

base_definition.h

class BaseDefinition {
public:
    
    //Virtual Extractor methods
    virtual void        extract_header(std::istream& file) = 0;
    virtual float       extract_data_point(std::istream& file) = 0;
    virtual bool        validate_descriptor()   = 0;
    virtual uint32_t    extract_total_points()  = 0;
    virtual double      extract_start_time()    = 0;
    virtual double      extract_end_time()      = 0;
    
    //Virtual Encoder methods
    virtual void        write_header(std::ostream& file, int &total_data_points, double &data_start, double &data_end) = 0;
    virtual void        write_data(std::ostream& file, float &t_value) = 0;

    //Basic methods
    virtual uint8_t get_start_pos() = 0;

};

peak_definition.h

#include <array>
#include <cstdint>


#include "base_definition.h"

namespace peak
{

#pragma pack(push, 1)
    struct  HeaderChunk {
        std::array<uint8_t, 24>     Descriptor; // Magic bytes 
        std::array<uint8_t, 36>     unused1;    // Not used currently 
        std::array<uint8_t, 4>      display1;   // Not used currently
        std::array<uint8_t, 4>      display2;   // Not used currently
        std::array<uint8_t, 4>      display3;   // Not used currently
        std::array<uint8_t, 4>      display4;   // Not used currently
        std::array<uint8_t, 4>      start;      // Used to compute Start 
        std::array<uint8_t, 4>      end;        // Used to compute End  
        std::array<uint8_t, 4>      unused2;    // Not used currently
        std::array<uint8_t, 4>      totalP;     // Actual data points present
        std::array<uint8_t, 4>      offset;     // Subtract this by total to get where data actually starts
        std::array<uint8_t, 2>      trail;      // End header 01 00
        std::array<uint8_t, 126>    pad;        // Pad of 00
    };
#pragma pack(pop)

#pragma pack(push, 1)
    struct DataChunk {
        uint8_t     chunk;
    };
#pragma pack(pop)

} //namespace peak

class PeakDefinition : public BaseDefinition
{
public:

    PeakDefinition();

    void        extract_header(std::istream& file) override;
    float       extract_data_point(std::istream& file) override;
    bool        validate_descriptor() override;
    uint32_t    extract_total_points() override;
    double      extract_start_time() override;
    double      extract_end_time() override;
    
    uint8_t     get_start_pos() override  { return data_start; } ;

    void write_header(std::ostream& file, int& total_data_points, double& data_start, double& data_end) override {};
    void write_data(std::ostream& file, float& t_value) override {};


private:
    
    const int sample_rate = 100; 
    const uint8_t data_start = 0xEC; //start of data

    peak::HeaderChunk header_buffer = {0};

    std::array<uint8_t, 24> magic_bytes = { 0x00, 0x01, 0x02, 0x03, 0x4, 0x00, 0x50, 0x00, 0x40, 0x00, 0x40, 0x00, 0x50, 0x00, 0x20, 0x00, 0x40, 0x00, 0x60, 0x00, 0x70, 0x00, 0x60, 0x00 };


};

peak_definition.cpp

#include <iostream>
#include <fstream>

#include "peak_definition.h"
#include "byte_operations.h"


PeakDefinition::PeakDefinition()
{
}

void PeakDefinition::extract_header(std::istream& file)
{
    file.read((char*)(&header_buffer), sizeof(header_buffer));
}

bool PeakDefinition::validate_descriptor()
{
    return header_buffer.Descriptor == magic_bytes;
}

uint32_t PeakDefinition::extract_total_points()
{
    return convertArray2int(header_buffer.totalP) - convertArray2int(header_buffer.offset);
}

double PeakDefinition::extract_start_time()
{
    double first = (int)header_buffer.start[0] * (double)sample_rate; //3,112,785 max
    double second = (int)header_buffer.start[1] * (double)sample_rate * 255; //793,760,175 max
    double third = (int)header_buffer.start[2] * (double)sample_rate * 255 * 255; //202,408,844,625 max

    return first + second + third;
}

double PeakDefinition::extract_end_time()
{
    return extract_start_time() + (convertArray2int(header_buffer.totalP) - convertArray2int(header_buffer.offset))* (double)sample_rate;
}

float PeakDefinition::extract_data_point(std::istream& file)
{
    peak::DataChunk buffer = { 0 };

    file.read((char*)(&buffer), sizeof(buffer));

    return (float)1.8 * buffer.chunk + 32;
}

csv_definition.h

#pragma once

#include <cstdint>
#include <string>

#include "base_definition.h"

namespace csv
{

    struct Row{
        std::string colA = "";
        std::string colB = "";
    };

} //namespace csv

class CsvDefinition : public BaseDefinition
{
public:

    CsvDefinition();
    void        extract_header(std::istream& input) override;
    float       extract_data_point(std::istream& file) override;
    bool        validate_descriptor() override;
    uint32_t    extract_total_points() override;
    double      extract_start_freq() override;
    double      extract_end_freq() override;

    uint8_t     get_start_pos() override { return 0; };

    void write_header(std::ostream& file, int& total_data_points, double& data_start, double& data_end) override;
    void write_data(std::ostream& file, float& db_value) override ;

private:

    void read_row(std::istream& file, csv::Row& row);

    csv::Row first_row_;
    float sample_rate_ = 0;
    const std::string separator = ",";
};

csv_definition.cpp

#include <iostream>
#include <fstream>
#include <string>
#include <sstream>

#include "csv_definition.h"
#include "byte_operations.h"

CsvDefinition::CsvDefinition()
{
}

void CsvDefinition::extract_header(std::istream& file)
{

}

bool CsvDefinition::validate_descriptor()
{ 
    int int_total = 0;
    double sample_rate = 0.0;
    bool assert = validate_csv_value(first_row_.colA, int_total) +
                  validate_csv_value(first_row_.colB, sample_rate);
    if (assert)
    {
        return true;
    }

    return false;
}

uint32_t CsvDefinition::extract_total_points()
{
    return std::stoi(first_row_.colA);
}

double CsvDefinition::extract_start_time()
{
    //TODO: Implement
    return 0.0;
}

double CsvDefinition::extract_end_time()
{
    //TODO: Implement
    return 0.0;
}

float CsvDefinition::extract_data_point(std::istream & file)
{
    //TODO: Implement
    return 0.0;
}

void CsvDefinition::read_row(std::istream& file, csv::Row &row)
{
    std::getline(file, row.colA, ',');
    std::getline(file, row.colB, ',');
}

void CsvDefinition::write_header(std::ostream& file, int& total_data_points, double& data_start, double& data_end)
{
    sample_rate_ = (data_start + data_end) / total_data_points;
    file << std::to_string(total_data_points) << separator << std::to_string(sample_rate_) << separator << "\n";
    file << std::to_string(data_start) << separator << std::to_string(data_end) << separator << "\n";
}

void CsvDefinition::write_data(std::ostream& file, float& db_value)
{
    file  << " " << separator << std::to_string(db_value) << separator << "\n";
}

byte_operations.h

#pragma once

#include <array>
#include <cstdint>
#include <sstream>

inline uint32_t convertArray2int(std::array<uint8_t, 4>& arr)
{
    return(arr[0] | (arr[1] << 8) | (arr[2] << 16) | (arr[3] << 24));
}

template<typename T> 
inline bool validate_csv_value(std::string& str, T &compare)
{
    std::stringstream convertor;
    convertor << str;
    convertor >> compare;
    if (convertor.fail())
    {
        return false;
    }

    return true;
}

Answer 1

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

Understand override

Using the override keyword after a member function declaration in C++ means that the function is intending to override a virtual member function in a base class. It's generally good practice to specify it when you are actually intending to override a function because it helps the compiler identify mismatches. However, it should not be used for functions that do not override virtual member functions. In this code, these two functions in csv_definition.h should NOT be overrides, because there is no corresponding virtual function in the base class:

double extract_start_freq() override;
double extract_end_freq() override;

Fix the many bugs

The get_start_pos() function in csv_definition.h can't compile because it returns data_start_ which is undefined. Similarly, extract_header does not do what it claims (it does nothing) and validate_csv_value() does not exist. The CsvDefinition::extract_start_time() and CsvDefinition::extract_end_time() override function declarations are missing from csv_definition.h. There is a long list. Turn on all available compiler warnings to get that list. This code does not seem really to be ready for a code review.

Use include guards

There should be an include guard in each .h file. That is, start the file with:

#ifndef PEAK_DEFINITION_H
#define PEAK_DEFINITION_H
// file contents go here
#endif // PEAK_DEFINITION_H

The use of #pragma once is a common extension, but it's not in the standard and thus represents at least a potential portability problem. See SF.8

Understand data initialization

The code includes this line:

peak::HeaderChunk header_buffer = {0};

I am guessing that this is intended to zero out the entire header_buffer but that is not what it does at all. In fact, it assigns the value 0 to HeaderChuck::Descriptor and leaves all of the other fields uninitialized.

Put your own #includes first

It is good practice to put your program's own #include files first. This allows you (and the compiler) to see if you have missed anything. For instance, the peak_definition.h file refers to std::ifstream but fails to #include <fstream>.

Provide complete code to reviewers

This is not so much a change to the code as a change in how you present it to other people. Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code. This affects not only code reviews, but also maintenance of the code in the future, by you or by others. One good way to address that is by the use of comments. Another good technique is to include test code showing how your code is intended to be used.