Help with semi-complex C++ assignment

Question

This is a really easy question I'm sure but I'd appreciate the help. :)

Here's my variable in the .h file:

map<int, map<int, map<int, CString>*>*> batch;

Here's me trying to assign a value:

((*((*(batch[atoi(transnum)]))[1]))[atoi(*docnum)]) = page;

I added some extra parentheses while trying to figure this out in order to make sure the derefs were being processed in the right order - unfortunately, it still doesn't work. My application just crashes when running this line. I have it wrapped in a try {} catch {}, but no exception appears to be thrown. I don't use C++ very often and am wondering whether someone can tell me what I'm doing incorrectly.

Here's the relationship I'm trying to model:

List of transaction numbers (integers), needs to be ordered by key.

For each transaction number, I have two types of documents, Payments and Invoices (buckets represented by 0 and then 1 respectively in my data struct above)

In each type bucket, there can be one or more documents, These documents need to be ordered by id (docid)

Each docid links to a string that consists of a comma-delimited list of files on the file system for processing.

If you think there's a better data structure to use, I'd be interested to hear it.

EDIT: I know there are many better ways to do this. The scenario was that I was handed a heap of horrible MFC-riddled C++ code and told to have something done yesterday. It basically boiled down to getting the data structure in there, loading it up and then outputting it somewhere else. I was just trying to pound it out quickly when I asked this question. I appreciate the design suggestions though.

Answer 1

The way std::map works is that it will allocate a node you are trying to reference if it does not exist yet. That means unless you are allocating your submap(s) and inserting them into your supermap(s), you're going to be given pointers to memory you don't own. At that point when you try to write to that memory you will crash.

Do the maps need to be heap allocated? If not you can change the type to:

map<int, map<int, map<int, CString> > > batch; // don't forget the spaces

and your call can be:

batch[atoi(transnum)][1][atoi(*docnum)] = page;

Answer 2

First, typedef these things, and it becomes much easier:

typedef std::map<int, CString> page_map;
typedef std::map<int, page_map> document_map;
typedef std::map<int, document_map> batch_map;

batch_map batch;

Note that you should almost always prefer he stack to dynamically allocating. Secondly, you're doing too much in a line!

int transNumber = atoi(transnum);
int docNumber = atoi(*docnum); // why is docnum a pointer?

batch[transNumber ][1][docNumber] = page;

Now if you need to debug you can easily check those values, and it's easier to see where you'd make mistakes.

I think with more information we could make this work a lot more simply. I can't think of why on Earth you'd need something like this.

Answer 3

That line is way too complex.

You need to break it up into smaller pieces, turning each piece into a named variable.

Answer 4

If you declare it:

map<int, map<int, map<int, CString> > > batch;//no asterisks!

you should be able to do this:

batch[atoi(transnum)][1][atoi(*docnum)] = page;

Answer 5

Just for fun: Why not make a collection of these?

typedef int transaction_key;
typedef int doc_id;

class Transaction
{
public:

    Transaction(transaction_key key) : m_key(key) {}

    AddPaymentDoc(doc_id, const std::string&);
    AddInvoiceDoc(doc_id, const std::string&);  
    // I'd probably have these methods return a unique ID actually, rather than 
    // create it yourself...  or they can return void and you pass in the doc id.


    // exception handling/other handling for attempting to reference using a bad id
    std::string GetPayment(doc_id);
    std::string GetInvoice(doc_id);

    std::map <doc_id, std::string> GetPayments() {return Payments;}
    std::map <doc_id, std::string> GetInvoices() {return Invoices;}

private:
    transaction_key m_key;
    std::map <doc_id, std::string> Payments;
    std::map <doc_id, std::string> Invoices;    
};

Answer 6

You're probably dereferencing a NULL or wild pointer at some point in that monstrosity. That kind of thing won't throw an exception, it will just cause a segmentation fault (or your platform's equivalent thereof).

Answer 7

Just going for a straight reading of what you are trying to model into simple data structures I ended up with this.

std::map is an ordered container so you end up with the orderings that you required. By avoidind the explicit use of pointers and allowing the container to manage the dynamic memory the model is simpler to use and less error prone.

If you have the potential for more document types than just payments and invoices then I might make the document type an enumeration and the transaction a map from document type to DocumentMap.

#include <map>
#include <string>

// Map of docid to comma separated string of files
typedef std::map<int, std::string> DocumentMap;

struct Transaction
{
    DocumentMap payments;
    DocumentMap invoices;
};

// map of transaction id to transaction contents
typedef std::map<int, Transaction> TransactionMap;

TransactionMap batch;

void foo(TransactionMap& batch)
{
    // ...

    batch[transno].invoices[docno] = page;

    // ...
}