Reputation: 5055
I'm writing a C program that needs good error handling. The code likes like this:
If(doWork("A")<0){
return -1;
}
If(doWork("B")<0){
undoWork("A");
return -1;
}
If(doWork("C")<0){
undoWork("A");
undoWork("B");
return -1;
}
return 0;
This code works but looks very messy, especially I have a long list of doWork(X)
to call. Is there a better and cleaner approach to handle error in this case?
Upvotes: 7
Views: 1432
Reputation: 1333
There is also another widely used approach based on a single pass loop that is clear and doesn't require goto. It implies though that Undo functions correctly handle both work that was done and that was not.
do
{
if(doWork("A")<0)
break;
if(doWork("B")<0)
break;
if(doWork("C")<0)
break;
return 0;
}
while(0);
undoWork("A");
undoWork("B");
undoWork("C");
return -1;
Upvotes: 0
Reputation: 13826
Being the same task doWork
, you can probably define a linked list or vector of jobs
and pass that as a parameter to doWork
, append the corresponding information to this list inside the function, and only call undoWork
once:
If(doWork("A", &jobs)<0){
return -1;
}
If(doWork("B", &jobs)<0){
undoWork(jobs);
return -1;
}
If(doWork("C", &jobs)<0){
undoWork(jobs);
return -1;
}
return 0;
This way, your logic will not become overly complicated, no matter the combination of jobs to be undone.
The advantage, compared to @twain249's solution, is that the function decides whether a job is added to the list or not, so you've got a nice isolation, modularity.
You can of course combine some form of an interable data structure with this, to further reduce the amount of repetitive code:
for(i=0; i < jobdata.size; i++) {
If(doWork(jobdata[i], &jobs)<0){
undowork(jobs);
return -1;
}
}
As you can notice, data structure design plays an important role in algorithm design, usually a much more important one than one usually thinks.
There could be thousands of jobs, the code will remain a four-liner.
Upvotes: 2
Reputation: 7640
If you don't have a super long list, you can approach it this way.
if (dowork("A") >=0) {
if (dowork("B") >=0) {
if (dowork("C") >=0) {
if (dowork("D") >=0) return 0;
undowork("C"); }
undowork("B"); }
undowork("A"); }
return -1;
Upvotes: 0
Reputation: 3971
Some people, especially beginner-to-intermediate programmers, have a very idiosyncratic reaction to seeing goto
in production code, but the usual idiom for sequential acquiring of resources and their intelligent release upon error is the following:
if(doWork("A") < 0)
goto errA;
if(doWork("B") < 0)
goto errB;
if(doWork("C") < 0)
goto errC;
/* success! */
return 0;
/* Error handling / releasing resources section */
errC:
undoWork("B");
errB:
undoWork("A");
errA:
return -1;
You will see plenty of examples in system code, e.g. in the linux kernel.
Upvotes: 7
Reputation: 5706
if it's possible to store all things you have to call doWork
on in an array then you could shorten the code significantly something like.
int i = 0;
int len = MAX_NUM; //set to the value of calls
int error = 0;
for(i = 0; i < len; i++) {
if(doWork(a[i]) < 0) {
error = 1;
break;
}
}
if(error) {
for(int j = 0; j < i; i++) {
undoWork(a[j]);
}
return -1;
}
Upvotes: 0
Reputation: 67345
Probably not. Newer languages like C++ and C# favor exceptions to help improve situations just like this.
Perhaps you could have a table that somehow indicated which tasks you've done and undo those. But I really think that would make your code more complex and not less.
Also note that, while there are some pretty strong feelings about using goto
, there are in fact times when that can simplify structures like this.
Upvotes: 0