std::cin and vector element edit

[ICantC]

I have 2 questions,

    int itemCount = 1;
    for (const auto &i : database)
    {
        std::cout << itemCount << "." << " " << i.name << std::endl
        ++itemCount;
    }
    fflush(stdin); std::cin.clear();
    int c; std::cin >> c;
    print("You are about to DELETE all details for "); std::cout << database[c].name;
    print("\n\nAre you sure you want to proceed? y/n\n\n\n> ");
    char warning; std::cin >> warning;
    if (warning == 'n') { return; }
    const std::vector<item>::iterator it = std::find(database.begin(), database.end(), database[c].name);
    database.erase(it);

in the above code, the program crashes when it gets to "std::cout << database[c].name" - but it works if I replace the 'c' with a number

the 2nd question is:

void item::create(const bool edit)
{
    print("Enter name for new item: "); fflush(stdin); std::cin.clear(); std::getline(std::cin, this->name);

    if (!edit)
    {
    database.push_back(*this);
    }
    else
    {
        const std::vector<item>::iterator it = std::find(database.begin(), database.end(), this->name);
        const int index = std::distance(database.begin(), it);
        database.insert(database.begin() + index, *this);
    }
}

say I had the list as:

1. Apple
2. Orange

and I edit apple and call it pear which executes the above code, I will be left with

1. Pear
2. Pear
3. Orange

even though I am over-riding the element position?

[ICantC]

I solved the first problem by convering the char to an int first

char c; std::cin >> c;
int c2 = static_cast<int>(c - '0');

[Handy Andy]

Hello ICantC,

Not sure what the bit of code is for in your second post, but the original code defined "c" correctly although it would work better as an "unsigned int" because as a subscript it can only be a positive number.

Line 2. I do not believe that you should or can use "const" in the range based for loop. I have not ever seen this and do not know if it can be done.

Line 7. You are mixing C and C++ code. The "fflush" may not work and there is better C++ code to clear the input buffer:
std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n'); // <--- Requires header file <limits>. . At this point there is no need for the "cin.clear()" because "cin" has not been used yet and there is nothing to clear.

Lines 9 and 10. What is "print" and which language is that from? Or am I missing something?

Line 13. Making the iterator a constant may not be a good idea especially if you need to change it later.

After that a few blank lines would make the code easier to read.

int itemCount = 1;

for (const auto &i : database)
{
	std::cout << itemCount << "." << " " << i.name << std::endl
		++itemCount;
}

fflush(stdin); std::cin.clear();

int c; std::cin >> c;

print("You are about to DELETE all details for "); std::cout << database[c].name;
print("\n\nAre you sure you want to proceed? y/n\n\n\n> ");

char warning; std::cin >> warning;

if (warning == 'n') { return; }

const std::vector<item>::iterator it = std::find(database.begin(), database.end(), database[c].name);

database.erase(it);

I would like to load this up for testing, but I am not sure if this is a function or part of main?

In the second bit of code my first impression is that you are making it harder than it should be.

"fflush" may not work on the "std::cin" input buffer the way you want. "std::cin.clear()" only clear the state bits on the stream. This has nothing to do with the input buffer that "std::cin" uses. Clearing the input buffer before a "std::getline()" is a good idea. Better to use the "std::cin.ignore()" I showed you after the last "std::cin >>", but can be used just before a "std::getline()".

Lines 11 and 12. making these "const" may not be the best idea. I can not say that I have seen this done before.

Line 13. What is the value of "index"? It looks like line 3 has changed "Apple" to "Pear", but is line 13 inserting the first "Pear" that you see or the second. And If you have changed the name why do you need to do the insert?

Some questions may be answered by seeing the whole program. As it is I have more questions than answers and no way to test the program.

Hope some of this helps,

Andy

[ICantC]

hi Andy, thanks a lot fo the reply. The const ranged based for loop seems to work ok, but thanks for clearing up the IO, I'll get that sorted.

If it will help you i've put the full program below, basically everything works as expected except the "void edit_item()" function where it seems to edit ok but I end up with a duplicate and I'm not sure why.

I'm new to programming so I know the below is very ugly, i'm just trying to see how stuff works. it's 3 files compiled with c++14 enabled

MAIN.CPP

#include "stuff.hpp"

std::vector<item> database;
const int MAX_ITEMS = 5;

void menu();
void load_data();
void display_items();
void edit_item();
void delete_item();

int main(int argv, char **argc)
{
    menu();
    return 0;
}

void menu()
{
    while (1)
    {
        system("cls"); std::cout << TITLE;
        std::cout << "1. View all items\n\n2. Add item\n\n3. Edit item\n\n4. Delete Item\n\n5. Save and exit\n\n\n> ";
        item i;
        char choice; std::cin >> choice;
        switch (choice)
        {
            case '1': load_data(); break;
            case '2': i.create(false); break;
            case '3': edit_item(); break;
            case '4': delete_item(); break;
            case '5': exit(EXIT_SUCCESS);
        }
    }
}


void load_data()
{
    // This code will load the existing items from a file eventually
    display_items();
}

void display_items()
{
    system("cls"); std::cout << TITLE;
    print("SELECT ITEM\n___________\n\n");
    int itemCount = 1;

    for (const auto &i : database)
    {
        std::cout << itemCount << "." << " " << i.name << std::endl << std::endl;
        ++itemCount;
    }

    print("\n\n\n> ");

    int c; std::cin >> c;
    if (c > MAX_ITEMS) { print("\n\nThis program currently supports a maximum of ");
    std::cout << MAX_ITEMS; print(" items."); Sleep(4000); return; }

    switch(c)
    {
        case 1: database[0].display(); break;
        case 2: database[1].display(); break;
        case 3: database[2].display(); break;
        case 4: database[3].display(); break;
        case 5: database[4].display(); break;
    }
}

void edit_item()
{
    system("cls"); std::cout << TITLE;
    print("SELECT ITEM FOR EDIT\n____________________\n\n");
    int itemCount = 1;

    for (const auto &i : database)
    {
        std::cout << itemCount << "." << " " << i.name << std::endl << std::endl;
        ++itemCount;
    }

    print("\n\n\n> ");

    int c; std::cin >> c;
    if (c > MAX_ITEMS) { print("\n\nThis program currently supports a maximum of ");
    std::cout << MAX_ITEMS; print(" items."); Sleep(4000); return; }

    switch(c)
    {
        case 1: database[0].create(true); break;
        case 2: database[1].create(true); break;
        case 3: database[2].create(true); break;
        case 4: database[3].create(true); break;
        case 5: database[4].create(true); break;
    }
}

void delete_item()
{
    system("cls"); std::cout << TITLE;
    print("SELECT ITEM FOR DELETION\n________________________n\n");
    int itemCount = 1;

    for (const auto &i : database)
    {
        std::cout << itemCount << "." << " " << i.name << std::endl << std::endl;
        ++itemCount;
    }

    print("\n\n\n> ");
    char c; std::cin >> c;
    int c2 = static_cast<int>(c - '0');

    if (c2 > MAX_ITEMS) { print("\n\nThis program currently supports a maximum of ");
    std::cout << MAX_ITEMS; print(" items."); Sleep(4000); return; }

    print("You are about to DELETE all details for "); std::cout << database[c2].name;
    print("\n\nAre you sure you want to proceed? y/n\n\n\n> ");

    char warning; std::cin >> warning;
    if (warning == 'n') { return; }

    const std::vector<item>::iterator it = std::find(database.begin(), database.end(), database[c2].name);
    database.erase(it);
    print("\n\nDeletion successful."); Sleep(2000);
}

ITEM.CPP

#include "stuff.hpp"

void print(const std::string &s)
{
  for (const char &c : s)
  {
      std::cout << c;
      // Sleep(20); for delayed rendering
  }
}

item::item()
{
    this->name = "N/A";
    this->username = "N/A";
    this->password = "N/A";
    this->custom_title = "N/A";
    this->custom_contents = "N/A";
}

item::~item()
{

}

void item::create(const bool edit)
{
    system("cls"); std::cout << TITLE;
    print("Enter name for new item: "); fflush(stdin); std::cin.clear(); std::getline(std::cin, this->name);
    print("Enter Username (if no username use \"N/A\"): "); fflush(stdin); std::cin.clear(); std::getline(std::cin, this->username);
    print("Enter Password (if no password use \"N/A\"): "); fflush(stdin); std::cin.clear(); std::getline(std::cin, this->password);
    print("Enter Custom Title (if no custom title use \"N/A\"): "); fflush(stdin); std::cin.clear(); std::getline(std::cin, this->custom_title);
    print("Enter Custom Contents (if no custom contents use \"N/A\"): "); fflush(stdin); std::cin.clear(); std::getline(std::cin, this->custom_contents);

    if (!edit)
    {
    database.push_back(*this);
    print("\n\nYour new item named "); print(this->name); print(" has been saved successfully.\n\n\n1. Back\n\n\n> "); Sleep(1);
    }
    else
    {
        const std::vector<item>::iterator it = std::find(database.begin(), database.end(), this->name);
        const int index = std::distance(database.begin(), it);
        database.insert(database.begin() + index, *this);
    }
}

void item::display() const
{
    system("cls"); std::cout << TITLE;
    print(this->name); print("\n");
    for (size_t i = 0; i < this->name.length(); ++i)
    {
        print("_");
    }
    if (this->username != "N/A")
    {
    print("\n\nUsername: "); print(this->username);
    }
    if (this->password != "N/A")
    {
    print("\n\nPassword: "); print(this->password);
    }
    if (this->custom_title != "N/A")
    {
    print("\n\n"); print(this->custom_title); print(": "); print(this->custom_contents);
    }
    print("\n\n\n1. Back\n\n\n> "); char c; std::cin >> c;
}

STUFF.HPP

#include <iostream>
#include <string>
#include <vector>
#include <algorithm>
#include <memory>

#include "windows.h"

#define TITLE "\tTEST VAULT\n\t___________\n\n\n"

void print(const std::string &s);

class item {
public:
    std::string name;
    std::string username;
    std::string password;
    std::string custom_title;
    std::string custom_contents;

    item();
    ~item();
    void create(const bool edit);
    void display() const;

    bool operator==(std::string n)
    { return this->name == n; } // for making std::find work
} ;

extern std::vector<item> database;

[Handy Andy]

Hello ICantC,

I will start with main.

The ".cpp" files should start with the "#include"s. I noticed that you put the "#include"s in the header file, more later. In place of the "Sleep" function I would add this to your "include" files:

#undef max  // <--- Needs to follow "window.h". If you do not include "window.h" yo will not need this.
#include <limits>
#include <thread>  // <--- For Sleep() replacement.
#include <chrono>  // <--- For Sleep() replacement. 

Then use:
std::this_thread::sleep_for(std::chrono::seconds(?)); // Requires header files "chrono" and "thread" in place of "Sleep()". Where the ? is the number of seconds to pause. This is better because "Sleep()" may limit your program or at least your code to Windows computers.

The functions that follow main should also include the "Print" function from the "Item.cpp" file, more on this later.

The "menu" function looks OK for now. I like to print the menu and enter a choice then check to see if it is a valid choice before returning the choice back to main. Then I would use the switch in main to control the program flow. I have see controlling the program from a function like "menu" cause problems when calling and thus returning from functions. For case 5 I would rather return to main and exit the program from there.

For now the "display_items" function looks OK.

The "edit_item" function looks OK for now.

The "delete_item" function is OK up to:

char c; std::cin >> c;
int c2 = static_cast<int>(c - '0');

which should be written as:

int c;

std::cin >> c;

Better to put these on two separate lines so you do not have to hunt for the "std::cin". As you have written it the "std::cin" is easy to miss. Defining "c" as an "int" will eliminate your line 2 and require a change in the first "print" line after the if statement.

All this is what I see in "main" before I actually run the program.

Now "Stuff.hpp":

I like your use of ".hpp" I too started using this several months ago.

First the file name "Stuff.hpp" should be "Item.hpp" as this file should only include the class definition.

The "#define" and the prototype for "print" really should not be there. I like to put things like this in a file I call "proto.hpp". I would also include the last line of this file in "proto".

Being a class definition I tend to use the class name as the file name and the same name for the ".cpp" file.

Inside your class you have made everything "public". This defeats the point of the class. Might as well just make it a struct which is public to start with. The variables should be in the "private" section and everything after the variables in the "public" section. Other than having things in the wrong sections the class looks good.

To the "Item.cpp" file:

The "print" function should be in main not here as this file should only deal with definitions of the class member functions.

In the ctor "this->aVariable" the "this->" is not needed. As a member function of the class this function already have access to the "private" variables of the class, but since all the variables are "public" everything has access to them it makes "this->" even more worthless. Not to say that "this->" does not have any use, there are some functions like overloaded operators that can use "this->". When a "std::string" is defined it is empty to begin with. There is no need to initialize unless you want it to contain some words or sentence that you want to use later. I would avoid setting these strings in the ctor with "N/A" as it could be a potential problem later.

In the "create" function the use of "fflush" may not work. I would use the C++ version:
std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n'); // <--- Requires header file <limits>. This will clear the input buffer of whatever may still be in there. The "std::cin.clear()" only resets the state bits on the stream should it fail like cin expecting a number in a numeric variable and getting a letter from the user. This will cause the stream to fail requiring the need for "std::cin.clear". Using the "ignore" before a "std::getline()" may or may not be needed. Sometimes you have to try the program with and without the "ignore" to see if it is needed.

Using the "fflush" and "std::cin.clear()" before every "std::getline()" is not needed. Maybe before the first "std::getline()".

The if/else statements at the end. Since this is a create function the if part should be there, If(..) and opening along with the closing {}s are not needed. The else part should not even be in this function. The insert part should be in an insert function. Because it is here you are inserting something into the vector that should not be done. That is why you see an extra "Peach" as yo described earlier.

Last function "display":

Here as with other places in your program a few blank lines make the code easier to read.

Again "this->" is not needed for the reasons I mentioned earlier.

Should you choose to not initialize your variables in the ctor, then your if statements would change to this: if (name.size() > 0) or if (name.length() > 0) would work fine.

What should be the last line of the function char c; std::cin >> c;, not hidden at the end of the print function call has no use. Is there a point to this or something you missed removing? Or was it a way to pause the program?

That said I want to thank you for not using the line using namespace std;. It is refreshing to see someone who has actually learned something. Now we just need to clean up the small things that you are not use to yet.

Hope that helps,

Andy

[ICantC]

Thanks very much for the detailed reply, I feel like i've learnt a fair bit. I've made the changes you said and it's all going well. Although I didn't seem to have to put #undefmax even though I was still including windows.h for the "system("cls")" function to clear the screen (i've replaced sleep with the this_thread delay as you mentioned)

Thanks again