1

I am new in c++, I am trying to make a very simple CRUD program.

In this example, a client bought many things in a store, and this store has the information about the things that this client bought. I called it Inventory.

The store, wants to print a Report for every client. In the code below, the main just has one Inventory, just for sample.

The problem is: When I want to print the Report, I have to get the data from the client, but without lose encapsulation. I mean, I want that no class can modify the content of the inventory.

What I am trying to do is convert the map into a vector (I need something to sort the data) and pass this vector (dinamically allocated). I am allocating this vector at the class Inventory but who is deleting is the class Report, this is not the correctly way to do things, but I do not know how to pass this information without doing like this.

Anyway, the report class can get the pointer to a Book, and use its set function or point to other Book. Again, I do not know how to do it correctly.

Can someone give me a tip what I have to do in this case ?

Thanks.

Sorry for the long code.

Main:

int main(void)
{
    Inventory i;
    Report r(i);

    i.addBook("Foo Bar I");
    i.addBook("Foo Bar II");

    r.generateReport();

    return 0;
}

Class Report in .h:

class Report
{
private:
    Inventory* i;
public:
    Report(Inventory& i);
    void generateReport();
};

Class Report in cpp:

Report::Report(Inventory& i)
{
    this->i = &i;
}

void Report::generateReport()
{
    ofstream out ("Report.txt");

    out << "Books: " << endl;

    vector<pair<int, Book *>> * b = i->getBooks();

    for(pair<int, Book *> p : *b)
    {
        out << p.first << ": " << p.second.getName() << endl;
    }
    out << endl;

    delete b;

    out.close();
}

Class Inventory in .h:

class Inventory 
{
private:
    map<int, Book *> books;

public:
    void addBook(int code, const string& name);
    vector<pair<int, Book *>> * getBooks();
};

Class Inventory in .cpp:

void Inventory::addBook(int code, const string& name)
{
    books.insert(pair<int, Book *>(code, new Book(name)));
}

vector<pair<int, Book *>> * Inventory::getBooks()
{
    return new vector<pair<int, Book *>>(books.begin(), books.end());
}
Christophe
  • 68,716
  • 7
  • 72
  • 138
ViniciusArruda
  • 970
  • 12
  • 27
  • 1
    Please post [a Minimal, Complete, and Verifiable example](https://stackoverflow.com/help/mcve) – Paul Evans Dec 03 '15 at 19:44
  • have you heard of setter/getter methods? – 463035818_is_not_an_ai Dec 03 '15 at 19:46
  • Yes, but I will make a getter for a map ? – ViniciusArruda Dec 03 '15 at 19:47
  • I dont know what you will do, but to access private data one usually writes a getter function, i.e. a method that returns a copy to the private variable. This ensures encapsulation, as modifiying the copy does not change the private member. – 463035818_is_not_an_ai Dec 03 '15 at 19:49
  • 2
    C++ is not Java. You should reduce, if not eliminate, all of those calls to `new`. Imagine if `getBooks` (for example) were called in a loop. – PaulMcKenzie Dec 03 '15 at 19:50
  • 2
    Prefer to return a const reference instead of a copy. Your current code is especially bad because the caller has to delete the results. – Kevin Dec 03 '15 at 19:50
  • @tobi303 OK, I agree, but if my data is large to be copied many times ? In this case, I need to return the content, sort, and print int sorted order. – ViniciusArruda Dec 03 '15 at 19:53
  • @Kevin of course const ref can be nicer, but it can also be harmful if not used correctly – 463035818_is_not_an_ai Dec 03 '15 at 19:53
  • 2
    Anything can be harmful if not used correctly – Kevin Dec 03 '15 at 19:54
  • @PaulMcKenzie I am learning Java too, and I am getting confused. – ViniciusArruda Dec 03 '15 at 19:54
  • if your data is large then do as @Kevin suggested and return a const ref instead, then you just have to be careful not to use the ref after the referenced object has been destroyed – 463035818_is_not_an_ai Dec 03 '15 at 19:54
  • @X0R40 *I am learning Java too* -- As long as you don't try to learn C++ by using Java as a guide (and vice-versa). Also, why does your `Report` class take a pointer? Why not an Inventory object? Right now, your class would be cumbersome to use, knowing that the Inventory object has to be in scope of the object that it was assigned to. – PaulMcKenzie Dec 03 '15 at 19:56
  • @Kevin @tobi303 How can I use the const correctly ? The map should get `const Book *` ? – ViniciusArruda Dec 03 '15 at 19:56
  • 2
    It's extremely hard to learn C++ and Java at the same time. Despite some similarities in the syntax, they are VERY different languages. – Bo Persson Dec 03 '15 at 19:57
  • 1
    @PaulMcKenzie This is what I was trying to do. I was converting my code in Java to C++. I discovered that I have to do it again from scratch. – ViniciusArruda Dec 03 '15 at 19:58
  • 4
    @X0R40 *I was converting my code in Java to C++* -- Bad move. Rewrite the code in C++ using C++ paradigms, don't just "convert". That's why your design is littered with `new` calls. – PaulMcKenzie Dec 03 '15 at 20:00

2 Answers2

0

Your inventory class should have this interface:

class Inventory 
{
private:
    map<int, Book *> books;

public:
    using const_iterator = std::map<int, Book*>::const_iterator;

    void addBook(int code, const string& name);
    const_iterator begin() const { return books.begin(); }
    const_iterator end() const { return books.end(); }
};

no reason to create a copy of the map into a vector! that is inefficient and not reason for doing it.

Rewrite the generateReport to work with this interface as follows:

for(const auto &p : *i)
{
    out << p.first << ": " << p.second.getName() << endl;
}
out << endl;
simpel01
  • 1,792
  • 12
  • 13
  • Ok, I will pass the iterator. But I have to sort the data. That is the reason to convert the map into a vector. – ViniciusArruda Dec 03 '15 at 20:02
  • @X0R40 `map` is inherently sorted. Were you sorting based on something besides the code? – Mooing Duck Dec 03 '15 at 20:04
  • In this code, there's no reason for the map value to be a pointer as opposed to a value. – Mooing Duck Dec 03 '15 at 20:05
  • I think you need to reason in terms of objects. Is the `Inventory` supposed to be sorted, or is this a responsibility of the `Report` class? – simpel01 Dec 03 '15 at 20:05
  • I want to print the report sorted by the code and the name of the book. Using vector, I am using : `sort(b->begin(), b->end(), bookCompare);` , where `bookCompare` is my function to compare the element to be sorted. – ViniciusArruda Dec 03 '15 at 20:07
  • @simpel01 Is responsability of the Report class to sort. – ViniciusArruda Dec 03 '15 at 20:08
  • I would claim this is responsibility of the `Report` class, not the `Inventory`. So make a copy of the in the `Report` class (using the begin(), end() iterators). The iterators are const so nobody is allowed to modify the status of the `Inventory`. Incapsulation won't be violated. – simpel01 Dec 03 '15 at 20:10
0

With your approach, the trick to transform the map into a vector introductes only an additional dependency: the report has to know about the internals of the inventory.

I'd rather recommend to go the way of the vistitor design pattern: the goal is to separate the algorithm (producing the report, the visitor) from the datastructure you explore (the inventory and its items). 

class Item { ...};      // Your object structure 
class Book : public Item { ... }; 
class Inventory { ...}; 

class Visitor { ... };  // Algorithms that shall be executed on structure
class Report : public Visitor {...};

This could also simplify the implementation of your inventory, as you'd no longer need to foresee different containers for different kind of items (provided they all inherit from some common element base class).

Theres is plenty of litterature on this pattern. I recommend you Design patterns, elements of reusable object oriented software from Gamma & al: its the orginal textbook, and guess what, the demo code shows an inventory with a pricing visitor ;-)

Here a naive on-line example based on your problem, to illustrate how it could work.

Christophe
  • 68,716
  • 7
  • 72
  • 138