Salam All,
This is a c++ programming post/question. So, if you're not a programming geek, please ignore gracefully :-)
I'm looking for advice on choosing a simple & elegant programming style for a certain type of functions. I will first explain the function, and then list several solutions I've seen (and don't totally like so far), and then I'll keep it open for people to post back their suggested answers.
The Function (i want to write)
The function simply has several statements to execute in order. Each statement may return an error code. The statements must be called in order, and if any fails, none of the rest must execute. The function also has initialization & clean up code that must run (whether the statements fail or not).
How to write this function in an elegant readable style that would allow future additions & modifications easily?
Following are several solutions I've seen. However, there's something I do not like about each. My question is... do you know of different method that would avoid any of my dislikings?
Style #1, if-else-if, example
int TheElegantFunction () {
/*** init-code ***/
int rc = 0; //return value. 0 means no error
mutex.get();
/*** the statements s1, s2, & s3 ***/
if (0 != (rc == s1())) {
} else if (0 != (rc == s2())) {
} else if (0 != (rc == s3())) {
} else {
//we were successful
}
/*** clean up ***/
mutex.release();
return rc;
}
advantages:
1- no code repetition
2- one-exit-point for the function
3- compact code (each statement takes 1 line)
disadvantages:
1- not very readable (sequential code does not belong to the if-conditional-expression)
2- what to do with non-error-returning statements, or addign-statements
3- placing those statements in else {} will force creating several levels of nested else { if... }
Style #2, if-error, example
int TheElegantFunction () {
/*** init-code ***/
int rc = 0; //return value. 0 means no error
mutex.get();
/*** the statements s1, s2, & s3 ***/
rc = s1();
if (rc != 0) {
/*** clean up ***/
mutex.release()
return rc;
}
rc = s2();
if (rc != 0) {
/*** clean up ***/
mutex.release()
return rc;
}
rc = s3();
if (rc != 0) {
/*** clean up ***/
mutex.release()
return rc;
}
/*** clean up ***/
mutex.release();
return rc;
}
advantages:
1- readable
2- easy
disadvantages:
1- you have to repeat the clean up code for every single statement
2- multiple exit points for the function - can be confusing to follow the logic
Style #3, goto-solution, example
int TheElegantFunction () {
/*** init-code ***/
int rc = 0; //return value. 0 means no error
mutex.get();
/*** the statements s1, s2, & s3 ***/
rc = s1();
if (rc != 0) goto end;
rc = s2();
if (rc != 0) goto end;
rc = s3();
if (rc != 0) goto end;
end:
/*** clean up ***/
mutex.release();
return rc;
}
advantages:
1- readable
2- one function exit point
3- compact
4- no repetition
disadvantages:
1- the frowned-upoen, spagetti-programming, goto statement!
2- the goto can mask the logic and make the function not very readable - especially if there are loops
Style #4, my-dream-language, example
int TheElegantFunction () {
/*** init-code ***/
int rc = 0; //return value. 0 means no error
mutex.get();
conditional_block (rc == 0 /* exit block when expression doesn't hold */) {
/*** the statements s1, s2, & s3 ***/
rc = s1();
rc = s2();
rc = s3();
}
/*** clean up ***/
mutex.release();
return rc;
}
advantages:
1- (I think) readable & elegant & clear logic-flow
2- no code-repetition
3- one entrance, one exit for the function
disadvantages:
1- imaginary (maybe a nice C# v3.0 feature? hint hint ;-))
so what do you guys think? do you know of a nice style of write such common-functions?
style #5, the dummy-switch
ReplyDeleteint main ()
{
int rc=0;
printf ("init\n");
switch (rc) {
default:
rc = s1(); if (rc) break;
rc = s2(); if (rc) break;
rc = s3(); if (rc) break;
rc = s4(); if (rc) break;
}
printf ("clean-up, rc=%d\n", rc);
return rc;
}
Some languages implement application level transactions e.g. ColdFusion, what you could do is that you throw an exception when things go wrong, and have a central place that handles clean up and returning result.
ReplyDeleteBut in your case I think I'd go for this.
rc = st1();
if( !rc )
rc = st2();
}
...snip...
//single return & clean up point
mutex.release()
return rc;
I think this is the most readable, no code repition, but if you wanna go for something ugly but quite geeky, whyy don't you try the ternary oprator, one liner ;)
rc = ( rc = st1() ? rc : (rc = st()2 ? rc : st2() ) .... this is not exactly how it would look like you need to play with it for couple of minutes to get it to run I suppose.
But you need a big ass comment to explain what this LINE is about ;)
Style 1 -- ugh. The conditional looks much more complex than it really is.
ReplyDeleteStyle 2 -- double ugh. Repeated code everywhere is badness.
Style 3 -- is what I use when I write COM code where you must check HRESULT values on every call.
This is NOT spaghetti programming! The reflexive knee-jerk idea that all gotos are bad is just silly. Gotos CAN be used badly, but using them to always branch downwards to a single, well-defined label with clear semantics is an excellent idea. It is possible to write very clear code in this style even with gotos.
A fifth approach that works in C++ is to make a class MutexGetter which can be allocated on the stack. Something like this pseudo-c++:
class MutexGetter{
public: MutexGetter(Mutex * m){
m->Enter();
}
~MutexGetter() { m->Leave(); }
};
Now you just have
void myfunc()
{
MutexGetter mg(&mutex);
rc = call1();
if (FAILED(rc)) return rc;
rc = call2();
if (FAILED(rc)) return rc;
return call3();
}
and you're done. when mg goes out of scope on any return, its destructor is called and the mutx is released.
The disadvantage of this approach is that the fact that the mutx is released by this line of code:
}
which when you're just reading the code for the first time, is a little non obvious!
Thanks guys for the feedback. It seems that style #3 is the most logical. It uses goto's in an obvious and restricted manner. The other option would be to use c++ exceptions - which has its advantages and disadvantages - but it certainly adds a level complexity to the code.
ReplyDeletebtw, I like Ammar's geeky solution - but now, just imagine that you want to add a (cout << "trace info";) to the code, what do you do? :-)