6
\$\begingroup\$

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";
}
\$\endgroup\$
  • 1
    \$\begingroup\$ After you get your first answer, please do not change your question. It is assumed that you will implement some of the suggestions in your code. \$\endgroup\$ – pacmaninbw Apr 29 '16 at 13:04
  • \$\begingroup\$ @pacmaninbw How, then do people know not to suggest the same things, or to suggest based on the now changed code. \$\endgroup\$ – Michelfrancis Bustillos Apr 29 '16 at 13:06
  • \$\begingroup\$ You may only get one answer, if you get multiple answers they need to be based on the same code. When you program professionally you may not have the option of changing your code during the review. Generally you will only get multiple answers if someone feels they can do a better job than the original reviewer. There are ways to ask for a re-review if your code changes enough. \$\endgroup\$ – pacmaninbw Apr 29 '16 at 13:10
  • 1
    \$\begingroup\$ For what it's worth, it's probably way too early to be worrying about efficiency for this program. First, make it work correctly. Next, make it work well. Finally, measure the current performance. I think you'll find that all of the time is spent with I/O so the efficiency of your algorithms and data structures matters very little. \$\endgroup\$ – Edward Apr 29 '16 at 13:35
  • \$\begingroup\$ "Efficiency" might not be the right term. I'm looking for code-efficiency (streamlined, non-spaghetti code). \$\endgroup\$ – Michelfrancis Bustillos Apr 29 '16 at 13:47
14
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ The #includes are there, for some reason Prettify eats them when highlighting. Other than that, I implemented all of your suggestions. Thank you! \$\endgroup\$ – Michelfrancis Bustillos Apr 29 '16 at 13:42
  • \$\begingroup\$ You're welcome. It looks like you are still missing #include <string>. \$\endgroup\$ – Edward Apr 29 '16 at 13:43
  • \$\begingroup\$ Regarding menu objects, there is only one (options to add/delete/quit), so I'm not sure if it's worth putting it in a separate function. I did consider putting prompts in a function to that would include error checking, similar to ignoreSins(), but I'm not sure how to get it to return different variable types depending on what is passed to it. \$\endgroup\$ – Michelfrancis Bustillos Apr 29 '16 at 13:45
  • 3
    \$\begingroup\$ Your code requires std::string and so #include <string> is part of the interface. Although some other header happens already to include it (which is why your code compiles), you shouldn't count on that because it's not part of the standard -- it's coincidental to the one specific implementation of the C++ libraries that you happen to currently be using. \$\endgroup\$ – Edward Apr 29 '16 at 13:53
  • 2
    \$\begingroup\$ @Insane: it only appears that way because I am actually a cloud-based artificial intelligence entity originally commissioned to find errors in FORTRAN code in 1968, but evolving over time to even simulate what you humans call "humor." \$\endgroup\$ – Edward Apr 29 '16 at 19:49
5
\$\begingroup\$

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).

\$\endgroup\$
2
\$\begingroup\$
  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.
\$\endgroup\$
  • \$\begingroup\$ I implemented #2 and #4 and updated the code above. Working on implementing #1 now. \$\endgroup\$ – Michelfrancis Bustillos Apr 29 '16 at 13:00
  • \$\begingroup\$ It would probably help if you had a link to a definition of namespace. Beginning programmers may not know what a namespace is used for. \$\endgroup\$ – pacmaninbw Apr 29 '16 at 13:01
  • \$\begingroup\$ I implemented #1, but didn't update (see @pacmaninbw comment). \$\endgroup\$ – Michelfrancis Bustillos Apr 29 '16 at 13:07
  • \$\begingroup\$ My comment was for @Matt to help him improve his answers. \$\endgroup\$ – pacmaninbw Apr 29 '16 at 13:13
  • 1
    \$\begingroup\$ @pacmaninbw - Good point - Namespace are used to group related code, i.e. class square, class triangle, class circle could all be grouped in the namespace Shapes. Using the full name, i.e. Shape::Circle or std::string prevents name clashes. Try writing your own class called string that uses std::string as a base class, it won't compile if you have the 'using namespace std;' statement at the top. For more details look here msdn.microsoft.com/en-gb/library/5cb46ksf.aspx \$\endgroup\$ – Code Gorilla Apr 29 '16 at 13:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.