1
\$\begingroup\$

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

\$\endgroup\$
2
  • \$\begingroup\$ 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. \$\endgroup\$ Aug 12, 2021 at 17:22
  • 1
    \$\begingroup\$ 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? \$\endgroup\$
    – uli
    Aug 12, 2021 at 20:14

2 Answers 2

1
\$\begingroup\$

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.
};
\$\endgroup\$
3
  • \$\begingroup\$ 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. \$\endgroup\$ Aug 12, 2021 at 21:57
  • \$\begingroup\$ @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. ... \$\endgroup\$
    – JDługosz
    Aug 13, 2021 at 13:46
  • \$\begingroup\$ ... 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. \$\endgroup\$
    – JDługosz
    Aug 13, 2021 at 13:46
0
\$\begingroup\$

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, ...) {
\$\endgroup\$
1
  • \$\begingroup\$ 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 \$\endgroup\$ Aug 22, 2021 at 14:22

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.