Displaying a menu

Question

I created this for a project at class, and I already turned it in so I know it's too late for changing it and whatnot as it's the end of the semester, but I want to know from the community how well I did. Don't judge all the std:: stuff. I built this using using namespace std;.

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

using std::cout;
using std::cin;
using std::string;
using std::ifstream;
using std::iostream;
using std::endl;
using std::ofstream;

void openFile(){
    string getcontent;
    ifstream openfile ("namelist.txt");
    if(openfile.is_open())
    {
        while(getline(openfile, getcontent))
        {
            cout << getcontent << endl;
        }
    }
}

void addName(){
    string name;
    cout << "Please type a name you would like to add:";
    ofstream outfile;

    outfile.open("namelist.txt", std::ios_base::app);
    cin >> name;
    outfile << name << endl;
    cout << "Name added!" << endl;
    outfile.close();
}

int main(int argc, const char * argv[]) {
    char ch;

    while(1) {
        /// print the menu
        cout << "Menu Lab v1" << endl;
        cout << "--------------" << endl;
        cout << endl;
        cout << "A) Display Names in list" << endl;
        cout << "B) Add a name to a list" << endl;
        cout << "C) Exit" << endl;
        cout << endl;
        cout << "Enter your choice:";
        cin >> ch;
        cout << endl;

        /// do something based on the choice
        switch (ch)
        {
            case 'a':
            case 'A':
                openFile();
                break;
            case 'b':
            case 'B':
                addName();
                break;
            case 'c':
            case 'C':
                exit(1);
                break;
            default:
                cout << "You entered an invalid option" << endl;
        }
        cout << endl;
        cout << endl;
    }
    return 0;
}

Answer 1

Here are some things that may help you improve your code.

Eliminate unused variables

The program currently doesn't do anything with argc or argv, so you could simply use int main() instead. This would give a strong clue to the reader of the code that there are no command line options.

Use all required #includes

The code uses exit(1) but doesn't include the corresponding header. The code should have

#include <cstdlib>

And then use std::exit(1). Alternatively, since it's already in main, you could just use return 0 instead of exit.

Use string concatenation

The menu includes these lines:

cout << "Menu Lab v1" << endl;
cout << "--------------" << endl;
cout << endl;
cout << "A) Display Names in list" << endl;
// etc.
cout << "Enter your choice:";

Each of those is a separate call to operator<< but they don't need to be. Another way to write that would be like this:

cout << "Menu Lab v1\n" 
        "--------------\n\n"
        "A) Display Names in list\n" 
        // etc.
        "Enter your choice:";

This reduces the entire menu to a single call to operator<< because consecutive strings in C++ (and in C, for that matter) are automatically concatenated into a single string by the compiler.

Don't duplicate important constants

The filename is hardcoded right now (see next suggestion), but worse than that, it's done in two completely indpendent places. Better would be to create a constant:

static const char *FILENAME = "namelist.txt";

Consider the user

Instead of having a hardcoded filename, it might be nice to allow the user to control the name and location of the file. For this, it would make sense to use a command line argument and then pass the filename to the functions as needed.

Be consistent with file operations

In addName the file is explicitly close, but in openFile it is not. This is because in openFile, openfile is a local variable that gets destroyed when the function returns, so the file is automatically closed. Either is acceptable, but it would be a good idea to just do it one way consistently. Similarly, the code uses the idiomatic combination declaration and open for openFile but uses two steps in addName. The latter could be rewritten as:

ofstream outfile(FILENAME, std::ios_base::app);

Separate user I/O from program functions

In the addName function, there are really two things happening: getting the name from the user and then appending that name to the file. Instead of interleaving those operations, I'd recommend separating them into separate functions. This would allow for better error reporting if any of the operations fail (e.g. if the file can't be opened). One possibility:

void appendName(const char *filename, string &name)
{
    ofstream outfile(filename, std::ios_base::app);
    outfile << name << endl;
}
void addName(){
    string name;
    cout << "Please type a name you would like to add:";
    cin >> name;
    appendName(FILENAME, name);
    cout << "Name added!" << endl;
}

Add error handling

File operations can fail, so your program should both detect and handle such failures. The one place the program currently checks a file operation at the moment is, unfortunately, not actually useful because it has no effect. Specifically this:

if(openfile.is_open())
{
    while(getline(openfile, getcontent))
    {
        cout << getcontent << endl;
    }
}

operates just like this:

while(getline(openfile, getcontent))
{
    cout << getcontent << endl;
}

Consider improving names

I think addName is not a bad function name, but openFile is, since that function does more than simply open a file, and particularly because it also contains a variable named openfile (which is also a bad name in my opinion). Perhaps listNames would be a better name.

Answer 2

Beside the answer already made. I have one more thing. Dont use std::endl if you just want a newline because it also flushes the stream. Consider using instead a simple newline \n.