ToDo List Program Efficiency

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
#include
#include

using namespace std;

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

int main(){
string trash;
cout totalList;
vector names;

if (ifExists("lists.txt")){
names = pullFile("lists.txt");
}
cout", cin>>fileName, cout 0 && x ", cin>>choice, cout", cin>>choice, cout>newItem, cout>newItem, cout>itemNumber, cout totalList.size()){
cin.ignore();
cin.clear();
rewind(stdin);
cout>itemNumber, cout pullFile(string fileName){
vector 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 totalList, string fileName){
fstream ioFile;
ioFile.open(fileName.c_str(), fstream::out);

for(int c = 0; c totalList, string fileName){
cout

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 or `. 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 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& 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& 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.

StackExchange Code Review Q#127048, answer score: 14