5
\$\begingroup\$

My path to learning C++ has been a rough one. Currently, I'm writing a project and my knowledge is very outdated. While I can write code that accomplishes the tasks required, I'd like to improve and learn new aspects of the language.

So, I've written a class called Sequencer. It works. It's not great. It's not terrible. But it's based on old code. How would I improve the class with modern code?


#pragma once
#include <regex>
#include <string>
#include <experimental/filesystem>


class Sequencer
{
public:

    //////////////////////
    // CTors / DTor
    Sequencer(std::string const first_frame_in): first_frame_name(first_frame_in) 
    {
        bool result = std::regex_search(first_frame_name, first_frame_regex_results, match_pattern);
        prefix = first_frame_regex_results[1];
        std::string frame = first_frame_regex_results[2];
        ext = first_frame_regex_results[3];
        first_frame = std::stoi(frame);
        digit_padding = frame.length();
        current_frame = first_frame;
        out_prefix = "_signature";
        file_delim = ".";

        get_last_frame();
    }

    ~Sequencer() {}

    Sequencer(Sequencer const&) = delete;
    void operator=(Sequencer const&) = delete;


    bool get_next_frame(std::string &next_frame, std::string &name_out)
    {
        if (current_frame <= last_frame)
        {
            tmp_num = add_padding(current_frame);
            next_frame = prefix + file_delim + tmp_num + file_delim + ext;
            name_out = prefix + out_prefix + file_delim + tmp_num + file_delim + "ppm";
            current_frame++;
            return true;
        }
        else
        {
            name_out = "";
            next_frame = "";
            return false;
        }
    }


private:

    void get_last_frame()
    {
        bool test = true;
        std::string tmpname;
        unsigned int i = current_frame + 1;
        do
        {
            tmpname = "C:\\ProgramData\\NVIDIA Corporation\\CUDA Samples\\v9.0\\3_Imaging\\MovieHasher\\data\\" + prefix + file_delim + add_padding(i) + file_delim + ext;
            test = std::experimental::filesystem::exists(tmpname);
            if (test)
                i++;
            else
                i--;
        } while (test);
        last_frame = i;
    }

    std::string add_padding(int frame_num_in)
    {
        std::string outval = std::to_string(frame_num_in);

        int length = outval.length();
        while (length < digit_padding)
        {
            outval = "0" + outval;
            length = outval.length();
        }

        return outval;
    }

    std::string first_frame_name;
    std::regex match_pattern = std::regex("([a-zA-Z]*).([0-9]*).(\\w{3})");
    std::smatch first_frame_regex_results;
    std::string prefix, ext, out_prefix, tmp_num, file_delim;
    int first_frame;
    int last_frame;
    int digit_padding;
    int current_frame;

};

A sample main file to run the Sequencer class

////////////////////////
// INCLUDES

//// System
#include <iostream>
#include <string>

//// User Defined
#include "../../Sequencer.h"

int main(int argc, char **argv)
{
    std::string image_filename = "prefix.00000.ext";
    std::string out_filename = "prefix_signature.00000.ppm";
    Sequencer sequencer(image_filename);

    bool tmp = true;
    do
    {
        tmp = sequencer.get_next_frame(image_filename, out_filename);
        if (tmp)
            std::cout << image_filename << ", " << out_filename << std::endl;
    } while (tmp);

    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Just want to thank both reviewers for their earnest and thorough reviews of my code. Wish I could accept both answers. \$\endgroup\$ – Andrew Jul 28 at 4:40
4
\$\begingroup\$

Code:

  • // CTors / DTor: This comment doesn't add anything that we can't see from the code.

    Sometimes comments like this denote sections or regions of code in a file, but without consistently doing this for all the code, we don't know where the "constructor / destructor" section ends. I'd suggest just removing the comment.

  • Sequencer(std::string const first_frame_in): There's little benefit to making function arguments that are passed by value const. It doesn't affect the caller, and makes the function signature harder to read.

    Perhaps this should be a const& though?

  • When we have lots of member variables in a class, adding an m_ prefix on the names (e.g. m_current_frame) helps to distinguish them from local variables or function arguments.

  • first_frame_name, match_pattern, and first_frame_regex_results are all unused after the constructor, so they could just be local variables rather than members.

  • tmp_num and tmpname are not informative names. They should also be local variables since they are each used in only one function.

  • Presumably negative frame numbers can't exist. If this is the case we should be using an unsigned integer type, not a signed one.

  • We should check the result of the regex search and handle an error (since presumably this is going to be a user specified string).

  • The regex captures "zero or more" digits, but we don't check that we got a match before trying to convert the frame number. Perhaps we should require "one or more" digits / letters when matching.

  • Perhaps the regex pattern should use the specified file_delim too.

  • Using a chain of + operators for std::strings tends to create a lot of temporary strings. Using std::string::append is likely to be faster, especially if we reserve() the memory we need first.

  • We can add the padding digits in one operation by using std::string::insert.


Design:

  • I'd suggest grouping the information extracted from the first frame filename into a struct. We can then put the regex search and data extraction into a separate function, and more easily use the constructor initializer list.

  • Unless there's a particular reason for it, I wouldn't try finding the last frame inside the Sequencer class. It's probably simpler to restrict the "sequencer" to only generating filenames, and leave checking if the file exists for code that does actual processing.

  • Perhaps something like FrameFilenameGenerator might be a more accurate name than Sequencer?

  • We can effectively use the same code to generate the input and output filenames.

  • If we let the user keep track of the current frame, our generator can generate the filename for any requested frame, and doesn't have to keep track of the frame itself.


So applying the above points, we might get something like:

#include <regex>
#include <string>
#include <experimental/filesystem>

class FrameFilenameGenerator
{
public:

    // note: 
    // `first_frame_filename` isn't copied, so we only need a const&
    // `file_delim` is a "sink argument" (i.e. we store a copy in the class), so we take the argument by value and move it into place.
    FrameFilenameGenerator(std::string const& first_frame_filename, std::string file_delim = "."):
        m_file_delim(std::move(file_delim)),
        m_info(get_first_filename_info(first_frame_filename))
    {

    }

    // note: we should probably be more careful with the frame offset!
    // first_frame_number + frame_offset could overflow!
    std::string make_filename(std::uint32_t frame_offset, std::string const& output_suffix = "") const
    {
        auto result_size = 
            m_info.prefix.size() + 
            output_suffix.size() + 
            m_file_delim.size() + 
            m_info.digit_padding + 
            m_file_delim.size() + 
            m_info.extension.size();

        auto result = std::string();
        result.reserve(result_size);

        result.append(m_info.prefix);
        result.append(output_suffix);
        result.append(m_file_delim);
        result.append(frame_number_to_string(m_info.first_frame_number + frame_offset, m_info.digit_padding));
        result.append(m_file_delim);
        result.append(m_info.extension);

        return result;
    }

private:

    struct FirstFilenameInfo
    {
        std::string prefix;
        std::uint32_t first_frame_number;
        std::size_t digit_padding;
        std::string extension;
    };

    static FirstFilenameInfo get_first_filename_info(std::string const& first_frame_filename)
    {
        auto result = FirstFilenameInfo();

        const auto match_pattern = std::regex("([a-zA-Z]*).([0-9]*).(\\w{3})");
        std::smatch regex_results;

        if (!std::regex_search(first_frame_filename, regex_results, match_pattern))
            throw std::runtime_error("First frame filename does not meet the required format!");

        result.prefix = regex_results[1];

        if (regex_results[2].length())
        {
            result.first_frame_number = std::uint32_t{ std::stoul(regex_results[2]) };
            result.digit_padding = regex_results[2].length();
        }

        result.extension = regex_results[3];

        return result;
    }

    static std::string frame_number_to_string(std::uint32_t frame_number, std::size_t digit_padding)
    {
        auto result = std::to_string(frame_number);

        if (result.size() >= digit_padding)
            return result;

        auto const zeros_needed = (digit_padding - result.size());
        result.insert(result.begin(), zeros_needed, '0');

        return result;
    }

    std::string m_file_delim;
    FirstFilenameInfo m_info;
};

#include <iostream>

int main()
{
    std::string image_filename = "prefix.00030.ext";
    auto generator = FrameFilenameGenerator(image_filename);

    for (auto i = 0u; i != 20u; ++i)
    {
        auto in = generator.make_filename(i);
        auto out = generator.make_filename(i, "_signature");

        //if (!std::experimental::filesystem::exists(in))
        //  break;

        std::cout << in << " -> " << out << "\n";
    }

    return EXIT_SUCCESS;
}
\$\endgroup\$
4
\$\begingroup\$

Better use of standard library

Right now, you have:

std::string add_padding(int frame_num_in)
{
    std::string outval = std::to_string(frame_num_in);

    int length = outval.length();
    while (length < digit_padding)
    {
        outval = "0" + outval;
        length = outval.length();
    }

    return outval;
}

This strikes me as a fairly inefficient way to do the job. Lacking a specific reason to do otherwise, I'd probably use a stringstream:

std::string add_padding(int frame_num_in) { 
    std::ostringstream buffer;

    buffer << std::setfill('0') << std::setw(digit_padding) << frame_num_in;
    return buffer.str();
}

If you really want to do it as string manipulation rather than using a stream, you could eliminate a fair amount of work with a simple subtraction:

std::string add_padding(int frame_num_in)
{
    std::string outval = std::to_string(frame_num_in);

    int pad_len = std::max(0, digit_padding - outval.length());

    return std::string(pad_len, '0') + outval;
}

Use applicable member functions

In get_next_frame, I'd change this:

    else
    {
        name_out = "";
        next_frame = "";
        return false;
    }

..to use .clear() instead:

else 
{
    name_out.clear();
    next_frame.clear();
    return false;
}

Minimize Scopes

As a general rule, I'd prefer to give each variable as narrow a scope as possible. Right now, you have a number of class member variables that are really only used in a single function. In such a case, it's better to define them inside that function. If you can define them inside the body of a loop or if statement, that's even better.

This not only keeps outside code from "messing" with something it shouldn't, but also makes the code considerably easier to read, since you don't need to look through the whole file to find things.

Construct regexes carefully

Right now you have:

std::regex match_pattern = std::regex("([a-zA-Z]*).([0-9]*).(\\w{3})");

This has a bit of a problem: in a regex, a . means "match any one character". From the looks of the data you're using with the code, it seems nearly certain that you wanted to match only an actual .. You also allow each of the other sub-patterns to be empty, so (for example) this will match against a string like "[] ", which I'd guess really isn't intended.

Unfortunately, you haven't documented what you really intended to require, so I can't suggest one that I'm sure is better. It looks like in the end, you mostly just want a pointer to the first digit in the string though. If so, it might be easier to just use your_string.find_first_of("0123456789") and be done with it (unless you really need to verify the format of the input string, in which case I doubt the regex you're using suffices).

Avoid hard-coding where parameters make more sense

Most of this path:

tmpname = "C:\\ProgramData\\NVIDIA Corporation\\CUDA Samples\\v9.0\\3_Imaging\\MovieHasher\\data\\" + prefix + file_delim + add_padding(i) + file_delim + ext;

...should probably be supplied as a parameter to the program rather than hard-coded into the class--for most practical purposes, it restricts the code to running on your machine, for no particularly good reason.

\$\endgroup\$
  • \$\begingroup\$ Here's an example of the files I was meaning to match with regex: filename.00000.ext \$\endgroup\$ – Andrew Jul 28 at 4:15
  • \$\begingroup\$ I updated the '*' searches to '+' searches in the regex where applicable. Really amazing feedback. I wish I could accept both answers, and hope I have the benefit of your reviews again. \$\endgroup\$ – Andrew Jul 28 at 4:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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