4
\$\begingroup\$

As an old COBOL coder, all this OOP stuff is foriegn. Would someone please take a look at this program? I'm looking for suggestions for ways to improve this program. While it works, I'm sure there's many way to make it better, particularly in the areas of memory management, OOP methodology, and idioms. Any advice is welcome. Thanks!

Edit: Here's the complete listing:

// FILE:        translate.cpp
// STANDARD:    C++17
// AUTHOR:      Phil Huffman
//
// PURPOSE:
// This is a python program that reads a text file and writes a text file with
// a name derived from the name of the input file (e.g. d.txt -> d.OUT.txt).
// It reads the first data set, determines where the protein sequence is in the
// count. Then, in every subsequent data set, the program will only return data
// that is related to that count. This will replace data that does that does
// not precisely align with sequence positions from the first data set (whether
// they are a dash or a letter). The output file will have the same number of
// data sets as the input file. The proteins sequence of the first data set is
// unchanged. In the remaining data sets, each character of the sequence is
// unchanged if the corresponding character of the first sequence is a letter.
// Otherwise, it is replaced with the specified replacement character.
// If none is specified, the default '+' will be used.
//
// OVERALL METHOD:
// The general tasks are:
// 1. Parse command line building a vector of file names
// 2. Instantiate a DataSet object
// 3. Repeat step 2 until all file names have been used.
//
// CLASSES:
//  InputParser
//
//  DataSet
//
// FUNCTIONS:
//  None.
//
// DATA FILES:
//    plain text files as indicated in command line

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

class
InputParser {
    public:
        InputParser (int &argc, char **argv){
            for (int i=1; i < argc; ++i)
                this->tokens.push_back(std::string(argv[i]));
        }

        const std::string& getCmdOption(const std::string &option) const{
            std::vector<std::string>::const_iterator itr;
            itr =  std::find(this->tokens.begin(), this->tokens.end(), option);
            if (itr != this->tokens.end() && ++itr != this->tokens.end()){
                return *itr;
            }
            static const std::string empty_string("");
            return empty_string;
        }

        bool cmdOptionExists(const std::string &option) const{
            return std::find(this->tokens.begin(), this->tokens.end(), option)
                != this->tokens.end();
        }

    private:
        std::vector <std::string> tokens;
};

class
DataSet {
    public:

        DataSet(const std::string fname, const std::string out_id,
                char sub_char, int line_length) {

            this->ifn = fname;
            this->ofn = fname;
            this->oid = out_id + ".";
            this->sc = sub_char;
            this->ll = line_length;

            make_ofn();
            read_file();
            parse_raw();
            get_results();
        }

        void write_to_file() {
            long l;     // length of output line
            long b;     // start of output line in sequence
            DataSet_item dsi;   // current dataset item
            std::ofstream out;
            out.open(this->ofn);
            if(out.is_open()) {
                for(auto i = data.begin(); i != data.end(); i++) {
                    dsi = *i;
                    out << dsi.name << std::endl;
                    for(b = 0; b < dsi.seq.size(); b += this->ll) {
                        l = this->ll;
                        while(b + l > dsi.seq.size()) {
                            l--;
                        }
                        out << dsi.seq.substr(b, l) << std::endl;
                    }
                }
                std::cout << this->data.size() << " data set items written to "
                    << this->ofn << std::endl;
            } else {
                std::cerr << ofn << " was not opened." << std::endl;
            }
            out.close();
        }

    private:
        struct
            DataSet_item {
                std::string name;
                std::string seq;
            };

        int ll;                             // line length
        char sc;                            // substitution char
        std::string oid;                    // makes output file name different
        std::string ifn;                    // input file name
        std::string ofn;                    // output file name
        std::vector<DataSet_item> data;     // where the action is
        std::vector<std::string> raw;       // raw data from input file

        void read_file() {
            std::ifstream file(this->ifn);
            std::string str;
            while (std::getline(file, str)) {
                this->raw.emplace_back(str);
            }
        }

        void get_results() {
            DataSet_item dsi;
            dsi = this->data[0];
            std::string seq_0 = dsi.seq;
            for(long n = 1; n < data.size(); n++) {
                dsi = this->data[n];
                for(long i = 0; i < dsi.seq.size(); i++) {
                    if(!isalpha(seq_0[i])) {
                        dsi.seq[i] = this->sc;
                    }
                }
                data[n] = dsi;
            }
        }

        std::string get_name(std::string s) {
            int start = s.find('[');
            int end = s.find(']');
            int len = end - start;
            return ">" + s.substr(start + 1, len - 1);
        }

        void cleanup_seq(std::string &s) {
            long i = 0;
            while(i < s.size()) {
                if(s[i] == '\n') {
                    s.erase(i, 1);
                } else {
                    i++;
                }
            }
        }

        void make_ofn() {
            size_t pos = this->ofn.rfind('.');
            pos = (pos == std::string::npos) ? this->ofn.size(): pos + 1;
            this->ofn.insert(pos, this->oid);
        }

        void parse_raw() {
            DataSet_item dsi;
            dsi.name = "";
            dsi.seq = "";
            std::string line = "";
            for(auto e = this->raw.begin(); e != this->raw.end(); e++) {
                line = *e;
                if(line[0] == '>') {
                    augment_data(dsi);
                    dsi.name = get_name(line);
                    dsi.seq = "";
                } else {
                    dsi.seq += line;
                }
            }
            augment_data(dsi);
        }

        void augment_data(DataSet_item &dsi) {
            cleanup_seq(dsi.seq);
            if(!dsi.name.empty() && !dsi.seq.empty()) {
                this->data.emplace_back(dsi);
            }
        }
};

int
main(int argc, char **argv){
    InputParser input(argc, argv);
    if(input.cmdOptionExists("-h")){
        std::cout << "Usage: translate -s + -l 60 -o OUT <at least one file name>\n"
            << "\ts:\tSubstitution char, default = '+'\n"
            << "\t\tOnly first character will be used.\n"
            << "\tl:\tMax output length, default = 60\n"
            << "\to:\tOutput identifier, default = \"OUT\"\n"
            << "\t\tOne or more file names.\n"
            << "\t\tThese files must be simple text files.\n";
    }

    int lineLen = 60;
    const std::string &lineLength = input.getCmdOption("-l");
    if(lineLength.size() > 0) {
        lineLen = std::stoi(lineLength);
    }

    std::string out_id = "OUT";
    const std::string &oout_id = input.getCmdOption("-o");
    if(oout_id.size() > 0) {
        out_id = oout_id;
    }

    char sc = '+';
    const std::string &substitutionChar = input.getCmdOption("-s");
    if(substitutionChar.size() > 0) {
        sc = substitutionChar[0];
    }

    std::vector <std::string> fns;
    int i = 1;
    while(i < argc) {
        if(argv[i][0] == '-') {
            i++;
        } else {
            fns.emplace_back(argv[i]);
        }
        i++;
    }

    std::string ts;
    for (auto i = fns.begin(); i !=fns.end(); i++) {
        ts = *i;
        DataSet data(ts, out_id, sc, lineLen);
        data.write_to_file();
    }

    return 0;
}
\$\endgroup\$
3
  • 4
    \$\begingroup\$ Are you able to add a bit more context as to what this program is actually supposed to do, and edit the title to reflect taht? \$\endgroup\$ Jun 5, 2020 at 19:14
  • 1
    \$\begingroup\$ This is hardly written in OO style. What's the goal of the program? \$\endgroup\$
    – Mast
    Jun 5, 2020 at 19:32
  • 2
    \$\begingroup\$ I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to edit and give it a different title if there is something more appropriate. Also, the comment at the beginning of the code contains "This is a python program" yet it is C++ code with the C++ tag... \$\endgroup\$ Jun 5, 2020 at 20:54

1 Answer 1

3
\$\begingroup\$

General comments

It is rarely necessary to use the this keyword to access member variables (typically this is used to disambiguate between local variables/parameters and member variables that have the same name). So you can remove all of the this-> uses in your code.

Usually the class keyword is kept on the same line as the class name. Visually, putting it on a line of its own splits the declaration up unnecessarily and makes it harder for me to figure out what it is doing. The declaration of the local DataSet_item struct is even harder to figure out.

Use consistent spacing. You have const{ in some places (without a space; you can use const {). Elsewhere, you're breaking up type names with spaces (std::vector <std::string>) where normally there is no space between a template name and its parameters. There should be a space between keywords and the ( for the expression that follows, and a space between the ) and { for blocks.

InputParser class

This InputParser constructor should not take the argc parameter by reference. Just pass it as int argc.

The loop body can be simplified to tokens.emplace_back(argv[i]);. Or you can take advantage of one of vector's constructors with

InputParser(int argc, char **argv): tokens{argv + 1, argv + argc}
{
}

Usually returning a reference from a function is risky, because if the thing that is referenced is destroyed before the user of the return value is done using it you can access a dangling reference. getCmdOption would be safer if it returned a string, not a string &. It would then avoid the need for constructing a static string object to return if the option isn't found. The downside is having to construct extra string objects but these are relatively cheap.

Use the auto keyword for complicated variable types. In getCmdOption, you don't need to type out that whole iterator name (which is easy to get wrong). Just use auto itr = std::find(tokens.begin(), tokens.end(), option);. empty_string doesn't need to have that initial value passed, since the default constructor for a string constructs an empty string (static const std::string empty_string;). Although if you change the return type to a string you can dispense with that line and just use return {}; or return std::string{};.

Unless you're always working with a narrow editor, there's no real reason to split the return in cmdOptionExists onto two lines. (There is also some discussion on where that operator goes when splitting lines like this; I prefer to put it at the end of the previous line.)

DataSet class

There is no reason to make value parameters const, and you should make use of the constructor initializer list. Variables in the list should be listed in their declaration order, since that is the order the compiler will construct them.

DataSet(std::string fname, std::string out_id, char sub_char, int line_length):
    ll(line_length), sc(sub_char), oid(out_id + '.'), ifn(fname), ofn(std::move(fname))
{
    // ...
}

Declare variables as close as possible to their initial use. In write_to_file, l, b, and dsi can be declared later. The output stream can be opened in the constructor call, the for loop can use a ranged-based for loop, and you don't need to check for going past the end for substr. You can leave the std:: off of the endl uses due to Koenig lookup:

std::ofstream out{ofn};
if (out.is_open()) {
    for (const auto &dsi: data) {
        out << dsi.name << endl;
        for (std::size_t b = 0, len = dsi.seq.size(); b < len; b += ll) {
            out << dsi.seq.substr(b, ll) << endl;
        }
    }
} else // ...

You can omit out.close() since the destructor will do that for you.

In read_file, you can use raw.emplace_back(std::move(str)); to avoid making a copy of the string.

get_results is doing a bunch of work it doesn't need to. It could do with a better name, as any function name that starts with get can be expected to return a value in some way. It also assumes that data is not empty.

void get_results() {
    if (data.empty()) return;
    auto seq_0 = data[0].seq;
    for (std::size_t n = 1, sz = data.size(); n < sz; ++n) {
        auto &dsi = data[n];
        size_t i = 0;
        for (auto &ch: dsi.seq) {
           if (!isalpha(seq_0[i]))
               ch = sc;
        }
    }
}

get_name will have problems if one or both of the '[' or ']' characters are not found in the string.

cleanup_seq should make use of std::erase:

void cleanup_seq(std::string &s) {
    std::erase(s.begin(), s.end(), '\r');
}

make_ofn will misbehave if the filename does not have a period but a directory name earlier in the path does.

In parse_raw, you don't need to assign initial values to dsi.name, dis.seq, or line, since the default string constructor will create an empty string. And the for loop can use the ranged-based for loop mentioned earlier. Rather than assigning an empty string to dsi.seq to clear it, you can use dsi.seq.clear();.

The main function

When processing the "-h" option, you should abort execution (i.e., return) after displaying the help.

Your input parsing assumes that any flag option is followed by a parameter value. This is not error checked. The loop that processes these arguments can get out of sync for bad input (and adding a comment to the i++ when skipping a parameter would clear up initial confusion, since it looks like you're double incrementing. You already have all that data in input, so could you take advantage of that?

The final for loop can be a range-based for loop.

\$\endgroup\$
0

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.