1
\$\begingroup\$

The goal here is to only save (to file) the most recent Objects created if Object::maxMemoryOfObjects is exceeded.

#include <iostream>
#include <list>
#include <algorithm>
#include <fstream>
#include <map>

struct Object {
    const int ID;
    Object() : ID(++lastObjectID) {}
    static const int maxMemoryOfObjects = 200;
    static int totalMemoryOfObjects, lastObjectID;
};
int Object::totalMemoryOfObjects = 0, Object::lastObjectID = 0;

struct Location {
    std::list<Object*> objectsLyingAround;
    Location() {
        for (int i = 0; i < 12; i++) {
            Object* object = new Object;
            objectsLyingAround.push_back(object);
            Object::totalMemoryOfObjects += sizeof(*object);
        }
    }
};

struct Person {
    void visitLocation (const Location*) {}
};

std::list<Location*> itinerary;

void saveObjects() {
    std::ofstream outfile ("Objects.txt");
    std::map<int, Object*/*, std::greater<int>*/> objectMap;
    for (Location* x : itinerary)
        for (Object* o : x->objectsLyingAround)
            objectMap.emplace (o->ID, o);
    std::cout << "Objects being saved (from oldest to newest) IDEALLY are:\n";
    for (const auto& x : objectMap)
        std::cout << x.first << ' ';
    if (Object::totalMemoryOfObjects > Object::maxMemoryOfObjects) {
        std::cout << "\nAlert!  totalMemoryOfObjects > maxMemoryOfObjects!  Will save only the newest objects.\n";
        auto it = objectMap.begin();
        while (Object::totalMemoryOfObjects > Object::maxMemoryOfObjects) {
            Object::totalMemoryOfObjects -= sizeof(*it->second);
            std::cout << "totalMemoryOfObjects reduced to " << Object::totalMemoryOfObjects << std::endl;
            it = objectMap.erase(it);
        }
        std::cout << "Objects being saved (from oldest to newest) NOW are:\n";
        for (const auto& x : objectMap)
            std::cout << x.first << ' ';        
    }
outfile << objectMap.size() << std::endl;
for (const auto& x : objectMap)
    outfile << x.first << ' ';
}

void loadObjects() {
    int numObjects;
    std::ifstream infile ("Objects.txt");
    infile >> numObjects;
    // etc...
}

int main() {
    Person sam;     
    for (int i = 0; i < 5; i++) {
        Location* nextLocation = new Location;
        itinerary.push_back (nextLocation);
        sam.visitLocation (nextLocation);
    }
    saveObjects();
    loadObjects();
}

Output:

Objects being saved (from oldest to newest) IDEALLY are:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30
 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 5
7 58 59 60
Alert!  totalMemoryOfObjects > maxMemoryOfObjects!  Will save only the newest objects.
totalMemoryOfObjects reduced to 236
totalMemoryOfObjects reduced to 232
totalMemoryOfObjects reduced to 228
totalMemoryOfObjects reduced to 224
totalMemoryOfObjects reduced to 220
totalMemoryOfObjects reduced to 216
totalMemoryOfObjects reduced to 212
totalMemoryOfObjects reduced to 208
totalMemoryOfObjects reduced to 204
totalMemoryOfObjects reduced to 200
Objects being saved (from oldest to newest) NOW are:
11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37
 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60

But the way I'm doing it ironically adds more memory to the Object class (its ID data member). Is there a better way to do this? We assume here that the Objects are being saved and loaded many times until megabytes or even gigabytes of data are being stored in memory, at which a max should be enforced. Really old items that are lying around in Locations but never picked up will assume to be discarded first since they are probably forgotten anyway.

\$\endgroup\$
  • \$\begingroup\$ Why not use one of the cache classes that are already out there, instead of reinventing the wheel? stackoverflow.com/questions/122914/generic-cache-of-objects \$\endgroup\$ – Snowbody Mar 8 '15 at 3:38
  • \$\begingroup\$ Your main question seems to be "But the way I'm doing it ironically adds more memory to the Object class (its ID data member). Is there a better way to do this?". If you had to do it manually, how would you change the contents of the data saved to the file? \$\endgroup\$ – R Sahu Mar 8 '15 at 7:25
2
\$\begingroup\$

First: Crappy indentation makes it hard to read.

Your algorithm seems unnecessarily complicated. Why not work out how many items you want to serialize and just serialize those items.

 // Work out how many items we will need to drop.
 std::size_t  countOfItems      = objectMap.size();
 std::size_t  maxNumberOfItems  = Object::maxMemoryOfObjects/sizeof(Object);

 std::size_t  advance  = (countOfItems > maxNumberOfItems)
                             ? (countOfItems - maxNumberOfItems)
                             : 0;

 // Get an iterator to the first item
 // Then advance it forward the number of items we want t drop
 // These are the older items (as map is a sorted container).
 auto printer = objectMap.begin();
 std::advance(printer, advance);

 // Print all the items using std::copy and a stream iterator
 // May not work out the box for you but you should learn how to do this.
 std::copy(printer, objectMap.end(), std::ostream_iterator<Object>(outfile));

Memory Ownership

Your code is littered with pointers.
This is not a good idea in general (but hard to tell without more context in this case).

RAW Pointers are very rare in C++ as they do not convey object ownership. Usually pointers are managed by some owning object that manages their lifespan (as a result you never interact with them directly but via their owning object).

You should be using containers of object (not pointers) or if you have object hierarchies then use "pointer containers" that understand their content is a pointer and can act accordingly. If you don't have pointer containers available (they are in boost) then you should at least be using smart pointers to maintain the concept of ownership.

Looks like an error:

for (const auto& x : objectMap)
    outfile << x.first << ' ';
}

You are not serializing the object but only the ID. I think you meant:

for (const auto& x : objectMap)
    outfile << x.second << ' ';
}

But now you also need to define:

std::ostream operator<<(std::ostream& str, Object const& data)
{
   returns str << /* Serialize object */
}
std::istream operator>>(std::istream& str, Object& data)
{
   returns str >> /* De-Serialize object */
}

Looks like it might be an issue.

Object::totalMemoryOfObjects += sizeof(*object);

The size of an object in memory is always fixed. So you may as well count the number of objects. Note: sizeof() is computed at compile time not runtime. If you want to add up the number of bytes in things like strings you need to write your own method to do it.

But when you use operator<< the object is generally serialized and the number of bytes can vary (unless you write your object in binary fixed size format to the stream (but then you need to worry about machine peculiarities like endianness (so unless there is an extreme need for speed it is usually best to use a non binary format)).

Declare variables JUST before you use them

std::ofstream outfile ("Objects.txt");

// 500 lines of code not using outfile

outfile << objectMap.size() << std::endl;
for (const auto& x : objectMap)
    outfile << x.first << ' ';
}

Move the declaration of outfile to the point just before you tart using it. In C++ because of constructors/destructors the side affects may allocate resources. If you don't need them don't use them so wait until you need them.

It also has the benefit of making the declaration near the code that uses it so it makes it real easy to see the type of the object you are using (as it is just there with the code).

Encapsulation

struct Person {

You seem to be making all your objects publicly accessible. This does not help encapsulation. You should make all your data members private by default. Then provide methods (which are verbs) that act on the object.

class Object
{
    static int const maxMemoryOfObjects   = 200;
    static int       totalMemoryOfObjects = 0;
    static int       lastObjectID         = 0;

    static int allocateNextAvailableID()
    {
        return ++lastObjectID;
    }
    const int ID;
    public:
        Object()
            : ID(allocateNextAvailableID())
        {}

        // define an ordering on your objects.
        friend bool operator<(Object const& lhs, Object const& rhs)
        {
             // With an ordering you can use Object as the key.
             // in a sorted container.
             return lhs.ID < rhs.ID;
        }
};

int main()
{
    std::set<Object>     objectSet; // No need to allow access to the ID
                                    // to external entities that do not need to know.
                                    // provide the appropriate access method (in this
                                    // case `operator<()` that defines the ordering.
}
\$\endgroup\$
  • \$\begingroup\$ std::set<Object> can contain quite a lot of memory. How about instead std::set<Object*, ObjectIsOlder> where struct ObjectIsOlder { bool operator()(const Object* x, const Object* y) const {return *x < *y;} }; ? \$\endgroup\$ – prestokeys Mar 8 '15 at 19:26
  • \$\begingroup\$ "The size of an object in memory is always fixed. So you may as well count the number of objects." What if Object is polymorphic. Its derived types will have larger sizes than Object's size, won't they? So use object->size_in_bytes() instead, where Object::size_in_bytes() const is virtual? \$\endgroup\$ – prestokeys Mar 8 '15 at 20:22
  • \$\begingroup\$ Hate using pointers but its possible. I would use boost::ptr_set<Object>. A container the understands about pointers. \$\endgroup\$ – Martin York Mar 9 '15 at 2:51
  • \$\begingroup\$ The sizeof() operator is a compile time constant. It can not determine the size of a derived object at compile time (because it can not now the actual run time type). Yes you can use a virtual method (seems like a good idea). A bit overkill do you really need a specific byte size on your files? \$\endgroup\$ – Martin York Mar 9 '15 at 3:08

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.