5

I have a cheesesales.txt CSV file with all of my recent cheese sales. I want to create a class CheeseSales that can do things like these:

CheeseSales sales("cheesesales.txt"); //has no default constructor
cout << sales.totalSales() << endl;
sales.outputPieChart("piechart.pdf");

The above code assumes that no failures will happen. In reality, failures will take place. In this case, two kinds of failures could occur:

  • Failure in the constructor: The file may not exist, may not have read-permissions, contain invalid/unparsable data, etc.
  • Failure in the regular method: The file may already exist, there may not be write access, too little sales data available to create a pie chart, etc.

My question is simply: How would you design this code to handle failures?

One idea: Return a bool from the regular method indicating failure. Not sure how to deal with the constructor.

How would seasoned C++ coders do these kinds of things?

Dennis Ritchie
  • 1,281
  • 1
  • 10
  • 15

7 Answers7

4

In C++ exceptions is the way to report errors. BTW exception in initialization list CAN be handled.

A function-try-block associates a handler seq with the ctor-initializer, if present, and the function-body. An exception thrown during the execution of the initializer expressions in the ctor-initializer or during the execution of the function-body transfers control to a handler in a function-try-block in the same way as an exception thrown during the execution of a try-block transfers control to other handlers.

Good code usually should use minimum of try/catch blocks on most upper (thread) level. Ideally only one. This way, knowing that "everything throws", you have not think too much about errors and your normal scenario code flow looks clean.

giorashc
  • 13,691
  • 3
  • 35
  • 71
zen-cat
  • 425
  • 2
  • 9
2

A constructor is used for initializing object's internal state. Do not use it for making heavy operations like reading and processing files.

Use a method instead which will read the file and throw an exception (or return a boolean for success) incase an error occured. Catch this exception in your main flow and handle it as you see fit.

EDIT : If this is the whole class purpose than maybe ChessSales should only contain the data and you should use a factory class (or maybe static utility class) which has a method for reading a CSV file and returning a ChessSales object containing the relevant data read from the CSV file. This way you separate your data from the business logic (in this case reading and parsing the CSV file)

giorashc
  • 13,691
  • 3
  • 35
  • 71
  • But the cheese sales data *is* the object's internal state. Without this sales data, the object doesn't make any sense. Or rather, if that were the case, a "data not yet loaded" would have to be yet another possible reason for the failure of the regular non-constructor methods. Is that the way to go? – Dennis Ritchie Dec 02 '12 at 10:01
  • You can throw an exception from the constructor indicating reading failed but I do not like the idea of throwing exception during objects construction phase. – giorashc Dec 02 '12 at 10:02
  • +1 it is not a good idea to have lots of code in a constructor, when something goes wrong it is hard to know in which state the object is. – AndersK Dec 02 '12 at 11:00
2

Well, throwing exception is the obvious choice. This has a few issues in C++, because catching exceptions thrown by initalizer list constructors is not possible, leading to all kinds of trouble.

So you should actually provide both constructor which accesses the file and may throw exception, and a default constructor which leaves the object in "data not loaded state". This allows using your object safely as a member of other classes, while also allowing data to be loaded (or exception thrown) by another constructor.

Another choice is to have the data loading constructor to not throw exception, but to set object in invalid state if loading fails, and have other methods throw exception, and have getter for current state.

In any case, you need error/uninitialized state for your class, no safe way around that, I believe.

Edit by comments of @MathieuM.: one alternative to achieve the "uninitialized state" externally is to make it optional, most easily by using a pointer wrapper class. It is then safely initialized to NULL in initializer list, and the real initialization is attempted in constructor body, with whatever error handling. By choosing this, you can just have the constructor throw exception, and let users of class worry about it.

hyde
  • 60,639
  • 21
  • 115
  • 176
  • I will disagree on the "data not loaded state", because this completely changes the invariant of the class. If the user wants a possibly uninitialized class, let her choose between `boost::optional` and `std::unique_ptr`. – Matthieu M. Dec 02 '12 at 11:37
  • Yes, in principle. But C++ has its limitations, and one of them is initializer list exceptions. It means, the object really needs either state "not initialized yet" or "initialization failed", or it's not suitable to be used as member variable. Using some "optional" wrapper is one way to achieve this externally (even when it's not optional variable), other would be to use pointer member variable and create object in constructor body. No ideal solution exists (maybe in C++11?). – hyde Dec 02 '12 at 12:20
  • you can actually handle exceptions in an initializer list, but you should just let exceptions propagate anyway. If you cannot build one of the attribute, what is the point of building the whole object *partially* ? **Just let it fail**. – Matthieu M. Dec 02 '12 at 12:32
  • @MatthieuM. The other class may want to throw a different exception. It may be a class which does provide error state. It may want to retry file operation, because it is on unreliable network share. More imporantly, a member variable constructor throwing exception is an implementation detail, and sometimes the member variable exception is not compatible with public API even when designed from scratch, let alone if changing existing class. But, editing the answer by your comments, thanks for the insight! – hyde Dec 02 '12 at 12:43
  • @MatthieuM. Oh, and thanks for pointing out the try-catch syntax for initializer list (for ref: http://stackoverflow.com/questions/2441009), that would solve the public API issue. (Been doing C++ almost exclusively with Qt, which mostly avoids exceptions...). – hyde Dec 02 '12 at 13:01
  • truth to be told, I usually avoid function-scope try catch and prefer whipping up a helper function to build the object and deal with the possible exceptions there. Seem cleaner doing it one object at a time. – Matthieu M. Dec 02 '12 at 17:09
2

You are right to distinguish the two kinds of failure, indeed they are subtly different.


Failure in the constructor: The file may not exist, may not have read-permissions, contain invalid/unparsable data, etc.

Just throw an exception. Half-built objects are the shortest way to bug ridden programs.

A class should have an invariant that the constructor establishes and that all methods maintain. If the constructor cannot establish the invariant, then the object is unusable and the best way to report this, in C++, is to throw an exception so that the language ensures the half-built object will not be used.

If people suggest that you might want an invalid state, remind them of the Single Responsibility Principle: your class already has a specific responsibility (its invariant), those who wish for more can encapsulate it in a class dedicated to provide optionality. Off my head, boost::optional and std::unique_ptr are both excellent choices.


Failure in the regular method: The file may already exist, there may not be write access, too little sales data available to create a pie chart, etc.

Unfortunately, you failed to distinguish between two cases:

  • methods that only ever read the instance
  • methods that also modify the instance

For all methods, you need to choose your error reporting strategy. My advice is that exceptions are exceptional. If failure is deemed to be exceptional (network link down when it's up 99.99% of the time), then an exception is fine. On the other hand, if failure is expected, generally depending on the input such as a find method or in your case a write method to a specified file, then you want to give the user the chance to react appropriately.

There are at least two ways left, after exceptions have been ruled out:

  • returning a code (bool, enum) indicating whether the operation went well or not
  • asking the user to provide an error policy that will be invoked in case of issue

The error policy might be as simple as an enum (skip, retry-once, throw) or as complicated as a full blown Strategy with various methods.

Furthermore, nobody says that your method may only have one error reporting mechanism. For example, you might choose:

  • to invoke an error policy if the file already exists (user-provided after all, they might want to switch to a different name)
  • to throw if the disk cannot be accessed (the hardware is generally expected to work!)

Finally, on top of this, methods that also modify the instance have to worry about maintaing the invariant that the constructor established. If a method may screw up the invariant, then it's no invariant at all, and your users should quake in fear each time the class is used... The general wisdom is to perform all operations that might throw before starting modifying the object.

A simplistic (but so easy) implementation is the copy and swap idiom: internally copy the object, perform the operations on the copy, and at the end of the method swap the copy's state with the current object's state. If anything goes wrong the copy is corrupted and immediately discarded during stack unwinding, leaving the object untouched.

For more on this topic, you might want to read about Exceptions Guarantees. What I described is a typical implementation of the Strong Exception Guarantee method, similar to databases transactions (all or nothing).

Matthieu M.
  • 287,565
  • 48
  • 449
  • 722
0

This is how I handle errors: I log all errors in a log file, and create a simple to use function to report errors. If the program doesn't operate well, I open the log file and find the reason.

Regarding to how to know about errors as they occur: One of the reasons for bringin exceptions into C++ was to report errors in constructors. Since constructors don't return values, they can throw exceptions and report failures this way. Personally, I don't like this scheme so much, because it makes you put your code within try..catch. If you forget to do it, you might wonder why your program crashed..

So what I would usually do is this:

1) Let the constructor set a member variable for success/fail, depending on a successful operation of the constructor. I can later check it with something like if (myobj.constructor_ok()...). Notice that I didn't access the member variable directly.

2) I like very much returning true/false from methods, denoting success/fail whenever possible. This make the code very readble.

3) ..and the above log file.

Israel Unterman
  • 13,158
  • 4
  • 28
  • 35
0

I think you can get the information from the input file before you construct an object. For example, the code may be like:

if(!getInfoFromFile("cheesesales.txt", date, amount, kindOfCheese, money)){
    cout << "Failed to get information from file." << endl;
    return FALSE;
}
CheeseSales sales(date, amount, kindOfCheese, money);
cout << sales.totalSales() << endl;
sales.outputPieChart("piechart.pdf");

Then you can avoid handling errors in constructor.
Of course, throwing an exception is a solution too. But I don't like it because the exception in c++ is very complex. You may encountered lots of unimaginable problems.

amosJi
  • 46
  • 4
0

You missed an important detail when asking this question: what do you want to happen when the operation fails?

I would definitely throw an exception in the case of the constructor. I'd like to be able to write code just like what you wrote above; I don't want it to look like

CheeseSales sales("cheesesales.txt"); //has no default constructor
if (!sales.good()) {
    // Somehow handle things here.
}
// The if block has to break control flow or reinitialise sales, otherwise
// these next lines will break.
cout << sales.totalSales() << endl;
sales.outputPieChart("piechart.pdf");

Throwing means I can assume much stronger invariants in this case, and is thus a good thing.

Seeing as the output operation does not change the state of CheeseSales, throwing there wouldn't strengthen invariants as much. In that case, using a helper object in order to print the chart may be better:

ChooseSales sales("cheesesales.txt");
PdfStream chart("piechart.pdf");
chart << sales.asPieChart();

(PdfStream could just be std::ofstream, but perhaps you'd like to provide some more functionality.)

If the operation fails, it can put the chart object into an error state, which would correspond to how iostreams generally work.

Cactus Golov
  • 3,474
  • 1
  • 21
  • 41