File reader and writer

Question

I was told that the following code of mine was poorly written. How bad is it? Do you have any recommendations?

#include <iostream>
#include <fstream>
void ioMenu(std::ifstream&, std::ofstream&, std::string&, std::string&);
int ioMenuChoice();
void fileOpener(std::ifstream&, std::ofstream&, std::string&, int&);
void fileReader(std::ifstream&, std::string&);
void fileWriter(std::ofstream&);
void fileOpenAndRead(std::ifstream&, std::ofstream&, std::string&, std::string&);
void fileCreateAndWrite(std::ifstream&, std::ofstream&, std::string&);
void readAndWrite(std::ifstream&, std::ofstream&, std::string&, std::string&);
int main() {
 std::string fileName {};
 std::string reader {};
 std::ifstream inputFile {};
 std::ofstream outputFile {};
 ioMenu(inputFile, outputFile,fileName,reader);
}
void ioMenu(std::ifstream& inputFile, std::ofstream& outputFile, std::string& fileName, std::string& reader) {
 switch (ioMenuChoice()) {
 case 1:
 fileOpenAndRead(inputFile, outputFile, fileName, reader);
 break;
 case 2:
 fileCreateAndWrite(inputFile, outputFile, fileName);
 break;
 case 3: 
 readAndWrite(inputFile, outputFile, fileName, reader);
 }
}
int ioMenuChoice(){
 int choice {};
 
 std::cout << "1. Read from file\n2. Output to File\n3. Both\n4. Exit\n";
 std::cin >> choice;
 while (choice < 1 || choice > 4){
 std::cout << "\nInvalid input. Please re-enter a number from 1 to 3 : ";
 std::cin >> choice;
 }
 return choice;
}
void fileOpener(std::ifstream& inputFile, std::ofstream& outputFile, std::string& fileName, int choice) { // Grab / Open file
 // Grab file name
 std::cout << "Please enter the directory and file name : Example : C:\\Users\\Alex\\Desktop\\numbers.txt : ";
 std::cin >> fileName;
 
 if (choice == 1) {
 inputFile.open(fileName);
 while (inputFile.fail()) {
 std::cout << "Invalid input. Please re-enter the directory and file name : Example : C:\\Users\\Alex\\Desktop\\numbers.txt : ";
 std::cin >> fileName;
 inputFile.open(fileName);
 }
 }
 else {
 outputFile.open(fileName);
 }
}
void fileReader(std::ifstream& inputFile, std::string& reader) { // Read from file
 while (inputFile >> reader) { 
 std::cout << reader;
 }
 inputFile.close();
}
void fileWriter(std::ofstream& outputFile) {
 std::string outputString("");
 std::cout << "What would you like to write to the file? ";
 std::cin >> outputString;
 outputFile << outputString;
 outputFile.close();
}
void fileOpenAndRead(std::ifstream& inputFile, std::ofstream& outputFile, std::string& fileName, std::string& reader) {
 fileOpener(inputFile, outputFile, fileName, 1);
 fileReader(inputFile, fileName);
}
void fileCreateAndWrite(std::ifstream& inputFile, std::ofstream& outputFile, std::string& fileName) {
 fileOpener(inputFile, outputFile, fileName, 2);
 fileWriter(outputFile);
}
void readAndWrite(std::ifstream& inputFile, std::ofstream& outputFile, std::string& fileName, std::string& reader) {
 fileOpenAndRead(inputFile, outputFile, fileName, reader);
 fileCreateAndWrite(inputFile, outputFile, fileName);
}

edit : Heres the finished code, sorry. The main thing I am looking for is if I am using functions correctly ( Does each have a specific role? etc

• Sᴀᴍ Onᴇᴌᴀ: Welcome to Code Review! 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.
• uli: There are indeed several things amiss here. Which ones do you already know about? You mentioned one criterium, what does that mean and does your code fulfill it?

Answer 1

Well, the first thing that jumps out is that there is no const in any of the parameters.

void fileOpener(std::ifstream&, std::ofstream&, std::string&, int&);

for example, the 3rd and 4th parameters are "out" parameters? This indicates that the function will modify the string and the int in the caller's copy. Yet, it is void so why not provide a result in the normal way?

The lack of parameter names also makes it less clear what these functions are doing and how to use them.

 std::string fileName {};
 std::string reader {};
 std::ifstream inputFile {};
 std::ofstream outputFile {};

You don't need {} after all of these, as they have default constructors.

The main function continues with
ioMenu(inputFile, outputFile,fileName,reader);
and nothing else. So if these are all "out" parameters, what are you doing with the results?

---

Looking at the functions, the shared parameters are passed through and (possibly) updated by each individual function that could be called.

The set of functions should be written as a class, with those 4 values as instance data, instead.

E.g.

class Putter {
 std::string fileName;
 std::string reader;
 std::ifstream inputFile;
 std::ofstream outputFile;
 void fileOpener();
 void fileReader();
 void fileWriter();
 void fileOpenAndRead();
 void fileCreateAndWrite();
 void readAndWrite();
public:
 void run(); // something like that. What you have in `main` goes here.
};

• d8Alexander: Thank you. I am a pretty novice programmer, I now get that I should be using const in all my parameters passed by reference if I do not plan on modifying their values. To my understanding, all arrays are passed as a reference since its a pointer to the first element, so whenever I make a function parameter with an array, should I be putting a const infront of them? thanks.
• JDługosz: @d8Alexander It can be overwhelming to be a novice these days, I'm sure! There's something to be said about starting with primitive systems first. Keep it up. Re arrays: passing an "array" (really a pointer to the first element) is a C thing and not something you should normally be doing. Write your functions to take iterators and work with any collection class, not limited to arrays. ...
• JDługosz: ... OTOH, the most efficient internal code might indeed work directly with a contiguous sequence in memory; use gsl::span for that. And since I brought it up, be sure to study the "Standard Guidelines" document that this is a link into.

Answer 2

Code structure review

This code intends to support two operations based on command line input

1. Read file
2. Write file

File open is not a separate operation, because it is implicitly required for read and write. So there are three function required. One main and and two handler functions for switches. Based on code in question, I don't see any further refactoring required.

So simple design can be to create following command line options

-read file_path

-write file_path

// Assumes you don't want to return read contents, you just want to push them stdout
void readFile(const std::string& file_path){ 
}
// Assumes you don't write content from stdin to file.
void writeFile(const std::string& file_path){
}
int main(int argc, char**argv) {
 // Parse commandline to get mode and file_path. On error print usage help
 // Based on switch invoke readFile or writeFile 
 
 return 0;
}

Code review

// Make clear distinction between in, out and in-out parameter.
// In parameters by const refs and other two can be passes as refs.
void ioMenu(std::ifstream&, std::ofstream&, std::string&, std::string&);
int ioMenuChoice();
void fileOpener(std::ifstream&, std::ofstream&, std::string&, int&);
void fileReader(std::ifstream&, std::string&);
void fileWriter(std::ofstream&);
void fileOpenAndRead(std::ifstream&, std::ofstream&, std::string&, std::string&);
void fileCreateAndWrite(std::ifstream&, std::ofstream&, std::string&);
void readAndWrite(std::ifstream&, std::ofstream&, std::string&, std::string&);

// As per signature, this function should return int value.
// But there is no return statement in function body.
int main() { 

// Can file be read without opening?
void fileOpenAndRead(std::ifstream& inputFile, ...) { 

// And in function name is red flag.
// It indicates functions is doing more than one thing which is not good design.
// Refactor to ensure function does only one thing.
void readAndWrite(std::ifstream& inputFile, ...) {

• d8Alexander: Thanks. Should I put code from void ioMenu in main or is it okay if I leave it in this function? Is it considered bad practice? I prefer to have a void function that would normally have my code from main and just call it as it looks cleaner