I have a dilemma regarding my removing database function code.

Whenever I remove the database in vector with unique, I cannot think of writing the code that could fill the gap with the removed number (like I removed database ID3 and I want that the IDs of further databases would increment to have a stable sequence, so the database with ID4 would become ID3).

I also don't know how to decrement my static int counter.

File:

**void Database::RemoveDB()
{
    int det;
    cout << "Enter the ID of the base that you want to remove:\n";
    std::cout << "The bases are:\n";
    printids();
    cin >> det;
    auto iter = std::find_if(std::begin(DbMain),  std::end(DbMain), [&](Base& nb)
    { return nb.get_ID() == det; });
    if (iter !=  DbMain.end())
    {
        DbMain.erase(iter);
    }
}**
std::istream &operator>>(std::istream &re, Base &product)
{
    std::cout << "Enter the name of the base: \n";
    re >> product.name;
    std::cout << "Enter the city where it is located: \n";
    re >> product.city;
    std::cout << "Enter the country where it is located: \n";
    re >> product.country;
    std::cout << "Enter the number of guns held: \n";
    re >> product.rifles;
    std::cout << "Enter the number of tanks stationing: \n";
    re >> product.tanks;
    std::cout << "Enter the number of planes stationing: \n";
    re >> product.planes;
    std::cout << "Enter the number of launchers stationing: \n";
    re >> product.launcher;
    product.get_counter();
    product.ID = product.counter;
    return re;
}
void Database::printids()
{
    for (auto it = std::begin(DbMain); it != std::end(DbMain);   it)
    {
        std::cout << std::endl;
        printnames(std::cout, *it) << std::endl;
    }
}
std::ostream &printnames(std::ostream &pr, Base &pro)
{
    pr << "\nID:" << pro.ID << "\nName:" << pro.name;
    return pr;
}

Header file:

#ifndef OOPLABPROJECT_BASE_H
#define OOPLABPROJECT_BASE_H
#include <string>
#include <vector>
#include "Base.h"

class Base{
    friend std::ostream &tosave(std::ostream &, const Base &);
    friend std::istream &toim(std::istream &, Base &);
    friend std::ostream &printnames(std::ostream &, Base &);
    friend std::istream &operator>>(std::istream &, Base &);
    friend class Database;
public:
    Base() = default;
    Base(std::string n, std::string ci, std::string co, int ri, int tan, int pla, int lan); //read-only
    Base(std::string init); //initialisation

    const std::string &get_name() const { return this->name; }
    const std::string &get_city() const { return this->city; }
    const std::string &get_country() const { return this->country; }
    const int &get_rifle() const { return this->rifles; }
    const int &get_tanks() const { return this->tanks; }
    const int &get_planes() const { return this->planes; }
    const int &get_launch() const { return this->launcher; }
    const int &get_ID() const { return this->ID; }
private:

    std::string name;
    std::string city;
    std::string country;
    int rifles;
    int tanks;
    int planes;
    int launcher;
    static int counter;
    int ID;
    static int get_counter()
    {
        counter  ;
        return counter;
    };
};
std::ostream &tosave(std::ostream &, const Base &); //save data into txt file
std::istream &toim(std::istream &, Base &);// to read the data from txt file
std::ostream &printnames(std::ostream &, Base &); //used for edit and remove function in Database class
std::ostream &operator<<(std::ostream &, const Base &); //input attributes
std::istream &operator>>(std::istream &, Base &); //output attributes
////////////////////////
class Database {
public:
    Database() = default;
    void print_data();
    void read_data();
    void saveDB();
    void openDB();
    void EditDB();
    void RemoveDB();
    void printids();
    void compare();
private:
    std::vector<Base> DbMain;
};


#endif //OOPLABPROJECT_BASE_H

CodePudding user response：

The thing you're doing here is called a design anti-pattern: Some structural idea that you could easily come up with in a lot of situations, but that's going to be a lot of trouble (i.e., bad). It's called a singleton: You're assuming there's only ever going to be one DbMain, so you store the length of that in a "global-alike" static member. Makes no sense! Simply use the length of DbMain. You should never need a global or a static member to count objects that you store centrally, anyways.

You never actually need the ID to be part of the object you store – its whole purpose is being the index within the dBMain storage. A vector is already ordered! So, instead of printing your .ID when you iterate through the vector, simply print the position in the vector. Instead of "find the base with ID == N and erase" you could do "erase the (DbMain.begin() N) element" and be done with it.

CodePudding user response：

The problem in your design is that you seem somehow to associate the unique ID with the index in the vector, as you initialize the IDs with a static counter. This is not a good idea, since:

• you would need to renumber a lot of IDs at each delete and this would make it very difficult to maintain cross-references.
• you could have several databases each with its own set of ids.
• there is a risk that you'd not get the counter to a proper value, if you add bases without reading them.

Moreover, iterating sequentially looking for an ID is not very efficient. A more efficient approach would be to store your bases in a std::map: this allows to find IDs very efficiently, indexing by ID instead of sequential number.

Your only concern would then be to ensure uniqueness of IDs. Of course, your counter approach could work, if you make sure that it is updated whenever a new base is created, and that the state is persisted with the database in the text file in which you'll save all this. You just have to make clear that the IDs are not guaranteed to be sequential. A pragmatic way to ensure contribute to this understanding is to issue an error message if there is an attempt to delete a record that was not found.