ToDo List Program Efficiency

Question

I created a simple todo list program in C++ with the ability to store multiple lists, and add and delete list elements.

I'm looking for suggestions on how to make it more efficient, obvious features/checks missing, standard compliance, etc.

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

using namespace std;

vector<string> pullFile(string fileName);
void pushFile(vector<string> totalList, string fileName);
bool ifExists(string fileName);
void printList(vector<string> totalList, string fileName);

int main(){
    string trash;
    cout << "\033[2J\033[1;1H";
    string fileName;
    vector<string> totalList;
    vector<string> names;

    if (ifExists("lists.txt")){
        names = pullFile("lists.txt");
    }
    cout<<"File List: \n";
    if(names.size() <= 1){
        cout<<"No lists found.\n";
    }
    else{
        for(int c = 0; c < names.size(); c++){
            cout<<(c+1)<<". "<<names[c]<<"\n";
        }
    }
    bool flag = true;
    while (flag){
        cout<<"\nEnter the number of the list you would like to load or Enter file name for a new list.\n >", cin>>fileName, cout<<"\n";
        if(isdigit(fileName[0])){
            int x = fileName[0] - '0';
            if(x > 0 && x < names.size()){
                fileName = names[x-1];
                flag = false;
            }
            else{
                cout<<"Not a list number.\n";
            }
        }
        else{
            fileName = fileName + ".txt";
            names.push_back(fileName);
            flag = false;
        }
    }
    pushFile(names, "lists.txt");

    if (ifExists(fileName)){
        totalList = pullFile(fileName);
    }
    bool cont = true;
    while (cont){
        printList(totalList, fileName);
        int choice = 0;
        cout<<"What would you like to do?\n";
        cout<<"1. Add item.\n";
        cout<<"2. Remove item.\n";
        cout<<"3. Quit.\n";
        cout<<">", cin>>choice, cout<<"\n";
        while(cin.fail()){
            cin.ignore();
            cin.clear();
            rewind(stdin);
            cout<<">", cin>>choice, cout<<"\n";
        }
        if(choice == 1){
            string newItem;
            cout<<"Enter new item: ", cin>>newItem, cout<<"\n";
            if(cin.fail()){
                cin.ignore();
                cin.clear();
                rewind(stdin);
                cout<<"Enter new item: ", cin>>newItem, cout<<"\n";
            }
            totalList.push_back(newItem);
        }
        else if(choice == 2){
            int itemNumber = 0;
            cout<<"Enter item number to delete: ", cin>>itemNumber, cout<<"\n";
            while(cin.fail() || itemNumber < 1 || itemNumber > totalList.size()){
                cin.ignore();
                cin.clear();
                rewind(stdin);
                cout<<"Enter item number to delete: ", cin>>itemNumber, cout<<"\n";
            }
            totalList.erase(totalList.begin() + (itemNumber-1));
        }
        else if(choice == 3){
            cont = false;
            pushFile(totalList, fileName);
            cout<<"Exiting.\n";
        }
        else{
            cout<<"Invalid Input!\n";
            cin.clear();
            cin.ignore();
        }
    }
}

vector<string> pullFile(string fileName){
    vector<string> totalList;
    fstream ioFile;
    ioFile.open(fileName.c_str(), fstream::in);
    string bullet;

    while (!ioFile.eof()){
        getline(ioFile, bullet);
        if(!bullet.empty()){
            totalList.push_back(bullet);
        }
    }

    ioFile.close();
    return totalList;
}

void pushFile(vector<string> totalList, string fileName){
    fstream ioFile;
    ioFile.open(fileName.c_str(), fstream::out);

    for(int c = 0; c < totalList.size(); c++){
        if(!totalList[c].empty()){
            ioFile<<totalList[c]<<"\n";
        }
    }

    ioFile.close();
}

bool ifExists(string fileName){
    ifstream infile(fileName.c_str());
    return infile.good();
}

void printList(vector<string> totalList, string fileName){
    cout << "\033[2J\033[1;1H";
    cout<<"List: "<<fileName<<"...\n";
    for(int c = 0; c < totalList.size(); c++){
            cout<<(c+1)<<". "<<totalList[c]<<"\n";
        }
    cout<<"\n\n\n\n\n";
}

Answer 1

I see a number of things that may help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. Know when to use it and when not to (as when writing include headers). In this particular case, I happen to think it's perfectly appropriate because it's a single short program that and not a header. Some people seem to think it should never be used under any circumstance, but my view is that it can be used as long as it is done responsibly and with full knowledge of the consequences.

Make sure you have all required #includes

The code uses std::vector and std::string but doesn't #include <vector> or <string>. There are about four missing #includes by my count.

Be careful with signed and unsigned

In the current code, the loop integers c and x are both signed int values, but they're being compared with unsigned quantity names.size(). Better would be to declare them all as unsigned or perhaps size_t.

Use object orientation

Because you're writing in C++, it would make sense to have methods that operate on a class such as ToDo which would be a collection of std::string objects. Doing that would greatly clarify your code and simplify main.

Use const references where practical

The code currently declares the pushFile function like this:

void pushFile(std::vector<std::string> totalList, std::string fileName);

But this makes the code duplicate both the list and the file name. Instead, make these const references to indicated that neither item is changed:

void pushFile(const std::vector<std::string>& totalList, const std::string& fileName);

Think of the user

I don't know about you, but my todo list items tend to have more than a single word. Unfortunately, this program does not allow that. Consider using getline to get list items from the user.

Don't loop on eof

The code for pullFile includes this loop:

while (!ioFile.eof()){
    getline(ioFile, bullet);
    if(!bullet.empty()){
        totalList.push_back(bullet);
    }
}

This is almost always wrong. Instead, you could make this both simpler and more correct by writing it like this:

while (getline(ioFile, bullet)) {
    if(!bullet.empty()){
        totalList.push_back(bullet);
    }
}

Use more whitespace to enhance readability of the code

Instead of crowding things together like this:

ioFile<<totalList[c]<<"\n";

most people find it more easily readable if you use more space:

ioFile << totalList[c] << "\n";

Check for errors

The call fstream::open can fail. You must check to make sure that the call succeeded before further processing or your program may crash (or worse) when given malformed input or due to low system resources. Rigorous error handling is the difference between mostly working versus bug-free software. You should strive for the latter.

Use "range for" and simplify your code

If you're using C++11, you can use "range for" to simplify your code. (If you're not, you should switch compilers to one that supports C++11 and simplify your life!) For example, in pushFile() we might have this:

for (const auto &item : totalList) {
    ioFile << item << '\n';
}

Use RAII

RAII is Resource Acquisition Is Initialization, which is a very common idiom in modern C++. We can apply that to pushFile for instance, by creating ioFile and opening the file simultaneously:

std::fstream ioFile(fileName);

Now the entire function is much cleaner, shorter and easier to read:

void pushFile(const std::vector<std::string>& totalList, 
        const std::string& fileName)
{
    std::fstream ioFile(fileName);
    if (ioFile) {
        for (const auto &item : totalList) {
            ioFile << item << '\n';
        }
    }
}

Note that there is no explicit close() because this is automatically done by the destructor for ioFile.

Use a menu object or at least a common menu function

In a number of places in your code, you have something like a menu. Your code presents a prompt (a list of values) and then asks the user to pick one. Rather than repeating that code in many places, it would make sense to make it generic. Only the prompt strings actually change, but the underlying logic of presenting the choices and asking for input are all the same. It looks like you're a beginning programmer, and so perhaps you haven't learned about objects yet, but this kind of repeated task with associated data is really well-suited to object-oriented programming and that's something that C++ is very good at expressing.

Answer 2

Just a few more comments I haven't seen anybody mention.

Avoid magic constants

An obvious example would be: cout << "\033[2J\033[1;1H";. If you're going to use ANSI escape sequences, I'd recommend giving them meaningful names, then writing them out:

const std::string ClrScr = "...";
const std::string Home = "...";
// ...
std::cout << ClrScr;

Use meaningful names

A classic example is your variable named flag, which does nothing to tell the reader about what that flag really means or does.

Other examples are a little more subtle, where you've used names that are arguably meaningful, but don't quite reflect what they really mean. For example, your ifExists strikes me as a fairly poor name. It doesn't really check whether the file exists, but whether it's accessible, where are two entirely separate things. But see more about ifExists below.

Avoid race conditions

Using ifExists introduces an almost unavoidable race condition. The obvious example, would be if the user starts up your program, and at the same time, somebody else is deleting "lists.txt". Your program starts up, uses ifExists to check that "lists.txt" exists, which it does. Immediately afterward, the other user deletes it. Right after that, your code attempts to open "lists.txt", and (since ifExists already assured that this was possible) doesn't necessary react entirely sanely if that attempt fails.

It's much better to just attempt to open/read the file, and react appropriately if that fails. This way, you only assume the file exists if you have it open and ready to read (and the typical OS will assure that as long as it's open, it'll remain usable).

Answer 3

1. Don't do using namespace std;", the whole purposes of namespace is to help correctly identify something, which a using statement defeats.

2. This block of code

       cin.fail()){
       cin.ignore();
       cin.clear();
       rewind(stdin);
       cout<<">", cin>>choice, cout<<"\n";
   }
   

could be a function.

1. Use constant references as function parameters unless you are really altering them inside the function.
2. Try a switch statement instead of the if statement when looking at your menu choice, they can be faster sometimes.
3. Look at passing a single multiline string to cout rather than the 5 lines of menu you currently pass.
4. If you don't change it make it const, The compiler is almost certainly doing it anyway, but it will help readers understand the intent of the code.
5. You could look at commenting and refactoring the code, this will hopefully help identify bits that can be improved.
6. If you have Visual Studio, try the performance tools in there to identify the most heavily used bit of code and look at improving that.