Copy file with text replacement

Question

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();
}

Answer 1

• 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.)

Answer 2

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.