7
\$\begingroup\$

I am currently learning C++. I coded a tiny program for school and I wonder if there could be issues (like bugs or security issues) with it.
Any ways to make it have weird behaviours would be very appreciated, any suggestions to improve my code in terms or performance or readability are also welcomed.

Instructions

Given a file named filename, and two strings s1and s2, this program should create a new file named filename.replace in which all occurrences of s1 are replaced by s2.

  • code needs to compile with the -std=c++98 flag (so no C++11 standard or more)
  • I only can use the standard library's functions
  • no *printf, *allocor free
  • no C file manipulation functions, only C++
  • no STL, no containers nor <algorithm> header
  • no std::string::replace function

#include <iostream>
#include <fstream>

int main( int ac, char **av )
{
    if (ac != 4) {
        std::cout << "Invalid number of arguments" << std::endl;
        return -1;
    }

    std::string     infile_name(av[1]);
    std::fstream    infile(infile_name, std::ios::in);
    if (!infile.is_open()) {
        std::cout << "'" << av[1]
        << "': issue while opening the file" << std::endl;
        return -1;
    }
    std::cout << "'" << av[1]
        << "': successfully opened in read mode" << std::endl;

    std::string     outfile_name(av[1]);
    outfile_name += ".replace";
    std::fstream    outfile(outfile_name, std::ios::out | std::ios::trunc);
    if (!outfile.is_open()) {
        std::cout << "'" << av[1]
        << "': issue while opening the file" << std::endl;
        return -1;
    }
    std::cout << "'" << outfile_name
        << "': successfully opened and truncated in write mode" << std::endl;

    std::string line;
    std::string s1(av[2]);
    std::string s2(av[3]);
    size_t      pos;
    while (std::getline(infile, line)) {
        if (!infile.eof())
            line += '\n';
        do {
            pos = line.find(s1);
            if (pos == std::string::npos)
                outfile << line;
            else {
                outfile << line.substr(0, pos);
                outfile << s2;
                line = line.substr(pos + s1.size());
            }
        } while (pos != std::string::npos);
    }
    // not necessary to close the files manually as std::fstream objects are RAII
    // objects: https://stackoverflow.com/questions/4802494/do-i-need-to-close-a-stdfstream
    // infile.close();
    // outfile.close();
}
\$\endgroup\$
4
  • \$\begingroup\$ Is it guaranteed that the first string cannot span two lines, i.e. contain a newline? \$\endgroup\$
    – Davislor
    Mar 17, 2023 at 7:49
  • 3
    \$\begingroup\$ C++98 is a pretty weird restriction these days, and really inhibits what you'll learn. Not your fault of course, but still disappointing. \$\endgroup\$ Mar 17, 2023 at 7:52
  • \$\begingroup\$ @Davislor there are no other constraints than the ones i mentioned in the post so no \$\endgroup\$
    – BobDeTunis
    Mar 17, 2023 at 8:05
  • \$\begingroup\$ @TobySpeight I agree in principle, but they are allowed to use std::string, which is sufficient for this. Perhaps the prof will have them coding their own implementations of the data structures shortly. \$\endgroup\$
    – Davislor
    Mar 17, 2023 at 8:21

2 Answers 2

7
\$\begingroup\$
  • std::getline and std::string are in the <string> header, so we need to include that.

  • Rather than using std::fstream directly, we can use the specific types std::ifstream and std::ofstream for input and output files respectively.

  • Error messages should be sent to std::cerr, rather than std::cout. Progress messages should go to std::clog. (It may not matter here, but it's a useful habit to get into for writing programs where the standard output is piped to a file, or used as input by another program.)

  • We create the variable infile_name, but continue to use av[1] in many places. We should use infile_name throughout, since it's more meaningful.

    Note that the error message if we fail to open outfile shows the wrong file name. (We might have noticed if we had used the more meaningful name, instead of av[1] ;) ).

  • Variables should be marked const when we don't need them to be mutable (e.g. infile_name, outfile_name, s1, s2).

  • After each replacement, the line string is reassigned to a new string object: line = line.substr(pos + s1.size());. This is likely to be quite slow (as it will probably involve memory allocation).

    Note that std::string::find takes a second argument, which is the offset in the string at which to start searching. By using a second std::size_t variable to track the start point of the search (and adjusting the output code a bit), we can avoid the expensive substring operation.

    In modern C++, we could avoid the use of substr when outputting to the stream by using std::string_view, but that's not available in C++98.

  • The code that does the actual processing could be moved to separate function:

    void stream_replace(std::istream& in, std::ostream& out, std::string const& search_term, std::string const& replacement);

    This is neater - it gives this section of code clear inputs and outputs, and a meaningful name, and makes our main function shorter and more readable.

    It also makes the code more testable: we can pass file streams to this function, but we could also pass std::stringstreams. So we could write a bunch of test cases with various input strings, and check that the code gives the expected output in each case. This is called unit testing.

  • (Note that both std::string and the file streams are in the C++ standard library (often and somewhat erroneously called the STL - see the big purple box at the bottom of this page for details). std::string is definitely a container type. This may just be just a mistake in the instructions you've been given; if you're sure you can use std::string then I don't think you're doing anything wrong. If you're actually supposed to avoid std::string too, then you'll probably need to look into using a fixed size character array when processing the files.)

\$\endgroup\$
2
  • \$\begingroup\$ If the prof means the literal Standard Template Library, it does not include std::string, which was separate—or, interestingly, any of the hash tables, such as std::unordered_map, which were added later. \$\endgroup\$
    – Davislor
    Mar 17, 2023 at 15:05
  • \$\begingroup\$ I suspect she actually meant, “Any standard library container other than std::string,” but if I were giving an assignment like this, I would probably list the headers whose interfaces students are allowed to use. \$\endgroup\$
    – Davislor
    Mar 17, 2023 at 15:09
3
\$\begingroup\$

Adding to the excellent review by user673679. I am writing this review as ignoring the assignment constraints. Places where I violate the constraints are marked with [NA]

  1. Don't change names of common variables a.k.a argv, argc. Everyone recognises these variables.
  2. Don't do unnecessary indentations. There is no need to align variable names with spurious spaces.
  3. Prefer auto var = Type(arg); over Type var(arg). In the second version, it is easy to get into most vexing parse problem, if the constructor for Type does not take any parameters. Or outright forget the braces and have a uninitialized type.
  4. Never use command-line arguments aka argv throug out the code. Instead validate the arguments as early as possible and package them into proper types.
  5. [NA] Prefer std::string_view over std::string or const char * const

Weird edge case

Your method fails if s1 is a multiline string.

\$\endgroup\$
2
  • \$\begingroup\$ Regarding 3., the C++ Core Guidelines even prefers Type var{args} ES.23. \$\endgroup\$
    – Alex
    Mar 21, 2023 at 22:24
  • \$\begingroup\$ The core guidelines talks about {} vs () not auto var = Type(arg) vs Type var(arg). The core guideline prefers braced initialisation {} over calling the constructor () to avoid narrowing conversions. In 3, I am arguing for what the community calls 'Almost Always Auto'. These are two seperate ideas. Almost always auto makes your code uniform and makes it harder to not initialize a variable and has nothing to do with narrowing conversions. \$\endgroup\$ Mar 21, 2023 at 22:56

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.