8
\$\begingroup\$

I wrote this cat clone in C++. It does two things

  1. prints the content of file if only one file is given.
  2. concatenate and print contents of file if more than one file are provided.

I am especially concerned with the overloading of << for the class CLI. I mainly wanted to gain some abstraction with this.

Below is the code for the program:

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

// Represents the input provided by the user ..
class Input{
 std::vector<std::string> fileCollection;
public:
 std::vector<std::string> invalidFile;
 Input(std::vector<std::string> fileList){
   fileCollection = fileList;
 }
 std::string get_file_contents(){
  // return contents of each file..
  std::stringstream file_buffer;
  for (auto file : fileCollection){
   std::ifstream newFile(file);
   if (newFile.good()){
    file_buffer << newFile.rdbuf();
    newFile.close();
   } else {
    invalidFile.emplace_back(file);
   }
  }
  return file_buffer.str();
 }
};


// represents the whole app we are working on ..
class CLI{
 Input* newInput;
public:
CLI(std::vector<std::string> fileNames){
 newInput = new Input(fileNames);
}

 friend std::ostream& operator << (std::ostream& os, CLI* myCli){
  os << myCli -> newInput -> get_file_contents();
  if ((myCli -> newInput -> invalidFile).size() > 0){
   std::string msg;
   for (auto file : myCli -> newInput -> invalidFile){
    msg = "Error: " + file + ": No such file or directory\n";        
    os << msg;       
    }
   }
   return os;
  }
 };


int main(int argc, char** argv){
  std::vector<std::string> fileNames;
  for (int i = 1; i < argc ; ++i){
   fileNames.emplace_back(argv[i]);
  }
  auto myCLI = new CLI(fileNames);
  // calls the overloaded << operator
  // displays the content of files
  std::cout << myCLI;
 }
\$\endgroup\$

2 Answers 2

15
\$\begingroup\$

We never delete myCLI. It's not clear why we allocate this object in dynamic storage with new, rather than simply instantiating as a local.

We always return an exit status of 0 (success), regardless of whether the inputs were successfully read or the output successfully written.

It looks like we write error messages to the standard output stream. They should go to the error stream, std::cerr.

We don't need any classes for this simple utility. It seems massively overcomplicated to me. And inefficient, because we read each file entirely into memory before starting to write it.

A much simpler version doesn't need any user-defined classes at all:

#include <cerrno>
#include <cstdlib>
#include <cstring>
#include <fstream>
#include <iostream>
#include <ranges>

int main(int argc, char** argv)
{
    try {
        std::cout.exceptions(std::ios::badbit|std::ios::failbit);

        std::ifstream is;
        is.exceptions(std::ios::badbit|std::ios::failbit);

        for (auto filename: std::ranges::subrange(argv+1, argv+argc)) {
            is.open(filename, is.binary);
            std::copy(std::istreambuf_iterator<char>{is},
                      std::istreambuf_iterator<char>{},
                      std::ostreambuf_iterator<char>{std::cout});
            is.clear();
            is.close();
        }
    } catch (std::exception const& e) {
        if (errno) {
            std::cerr << strerror(errno) << '\n';
        } else {
            std::cerr << e.what() << '\n';
        }
        return EXIT_FAILURE;
    }
}
\$\endgroup\$
5
  • \$\begingroup\$ I removed the heap allocation and set those allocations to stack, returned 0 from the main. I am using a class because I have plans to extend this further for more features. I am concerned with the inefficiency now, any suggestions for avoiding copying by keeping the structure of the code same. Do I need to print as soon as I get the character, but how can I achieve this if I have a overloaded <<? \$\endgroup\$ Commented Dec 8, 2022 at 9:27
  • 2
    \$\begingroup\$ @AbhilekhGautam: Classes do NOT help adding features, magically. Especially when writing small utilities like cat, which are used in pipes, try to think in terms of pipes: what is the data-flow within the application? What steps are you executing? You need functions (steps), but not necessarily an orchestrator class. \$\endgroup\$ Commented Dec 8, 2022 at 18:30
  • \$\begingroup\$ Minor improvement, it's better to be clearer with the auto for entries you don't plan on modifying: for (const auto& filename : std::ranges::subrange(argv+1, argv+argc)) { \$\endgroup\$
    – mascoj
    Commented Dec 9, 2022 at 14:44
  • \$\begingroup\$ @mascoj, yes, const might be valuable there - but probably not & for a simple pointer. I think const char *const filename is probably better choice, as that shows we don't modify the string contents either. \$\endgroup\$ Commented Dec 9, 2022 at 14:55
  • \$\begingroup\$ @TobySpeight Yes you are right, when I saw subrange I assumed a wrapper would be used when iterating but that doesn't seem the case. \$\endgroup\$
    – mascoj
    Commented Dec 9, 2022 at 18:54
11
\$\begingroup\$

Style Comment:

The indent of one space is a bit small. More standard is 4 but please a minium of 2 spaces. Some of your functions are very crowded vertically. Some blank lines in logical places to help readability would definitely be appreciated.


Here is a simplification of loading the fileNames vector:

  std::vector<std::string> fileNames;
  for (int i = 1; i < argc ; ++i){
   fileNames.emplace_back(argv[i]);
  }

Could be written as:

  std::vector<std::string>  fileNames(argv + 1, argv + argc);

Don't dynamically allocate memory when an automatic variable will.

   auto myCLI = new CLI(fileNames);

Why not just:

   CLI   myCLI(fileNames);

It's shorter!


Memory management.

You do realize that this leaks:

 auto myCLI = new CLI(fileNames);

 // and this:
 newInput = new Input(fileNames);

Note: The problem is not only the loss of the memory, but the loss of not calling the destructor. You create objects, so they correctly clean up after themselves, but if the destructor is not called, they can't clean up their own mess.

The other problem is you don't have a destructor to clean up newInput. Every call to new should (MUST) be matched with a call to delete. Which is why we prefer you to use automatic variables rather than dynamic in the first place.

This is just sloppy. (Automatic fail in any C++ class).

Note: Modern C++ rarely uses new and delete at all. You should be using automatic objects by preference, if you do need to dynamically create objects then they should be managed via either smart pointers or containers. You only need to use new/delete if you are creating a new container or smart pointer or diving very deep into the abstraction.


Design: Why do you load the file content into memory, then print it out? This means your application is limited to files that are a fraction of the memory on your machine.

Sure; you could use an intermediate buffer to read large chunks from the file. BUT The std::fstream types already do this internally (so you are simply replicating their work if you did it again). Now I could see replicating this buffering if you are doing it as a class exercise but not in real life.

The simplest way to copy a stream to a stream is simply stream the buffer:

std::ifstream  src(fromFileName, std::ios::binary);

std::cout << src.rdbuf();

I would prefer this over using std::copy() as suggested by @Toby.


A Copy of @Toby's code with two minor changes I would do:

#include <cstdlib>
#include <fstream>
#include <iostream>
#include <ranges>

#include <cstring>
int main(int argc, char** argv)
{
    try {
        std::ifstream is;
        // I really like this.
        // Normally we don't do this but in this situation it is
        // a very clever way of handling the error cases.
        is.exceptions(is.badbit|is.failbit);
        std::cout.exceptions(is.badbit|is.failbit);

        // Clever use of ranges :-)
        for (auto filename: std::ranges::subrange(argv+1, argv+argc)) {
            is.open(filename, std::ios::binary);

            std::cout << is.rdbuf();
            
            is.close();
        }
    } catch (std::exception const& e) {
        std::cerr << e.what() << '\n';

        // Rather than return an error
        // I would re-throw the exception.
        throw;

        // This can provide more information to the host OS.
    }
}
\$\endgroup\$
10
  • 1
    \$\begingroup\$ Are you sure that rethrowing is right from main()? On my systems, the default exception handler will abort, causing a core dump. Which seems like overkill for the common case of file not found, or not readable. \$\endgroup\$ Commented Dec 8, 2022 at 17:15
  • 2
    \$\begingroup\$ And thanks for spotting the other new that I missed in that dense mass! \$\endgroup\$ Commented Dec 8, 2022 at 17:17
  • 1
    \$\begingroup\$ @TobySpeight My compiler does not have subrange yet. So I had to use a std::vector<std::string> filename(argv + 1 , argv + argc); then loop over that. Other wise the code seemed to work. \$\endgroup\$ Commented Dec 8, 2022 at 23:50
  • 1
    \$\begingroup\$ Agreed, when the error is caused by the programmer. Less so when it's due to the user or an anticipated environment problem such as trying to write to a full filesystem, because people don't generally want core dumps scattered all over their filesystems. If we can understand the cause then the code could choose whether or not to throw - but that's a whole new review then, I think! \$\endgroup\$ Commented Dec 9, 2022 at 8:10
  • 2
    \$\begingroup\$ @G.Sliepen I catch to force all destructors to be called. If an exception escapes main it is implementation defined if the destructors are called. By catching you force the destructors to be correctly called before you re-throw. \$\endgroup\$ Commented Dec 9, 2022 at 21:50

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.