> This is a plea for some advice on coding style from any of the experts
> on the list.  I have written some I2C drivers for use under Windows
> for a project, and whilst they are perfectly functional I feel that my
> coding style could definately be improved.  Now, a function should
> preferably have one entry and one exit.  However my code looks
> something like the function below.  Here I am calling several
> functions that return a boolean to indicate success or failure, and I
> want to exit the calling function if a failure is returned.
>
>
> bool _stdcall X9241Write(unsigned char address, unsigned char
> instruction) { /* Function:               X9241Write Arguments:
> address = slave address of device, instruction = instruction byte to
> send to pot Return:                 True if success, false if fail.
> Description:    Generic write command for instructions that required
> no additional data. */
>
>         unsigned char fullAddr;
>
>     fullAddr = (unsigned char)(X9241_ADDR | (address & 0x0F));
>
>     if(!I2CStart())
>         return false;
>
>     // send slave address
>     if(!I2CTransmit(fullAddr))
>         return false;
>
>     if(!I2CGetAck())
>         return false;
>
>         // send the MSB
>     if(!I2CTransmit(instruction))
>         return false;
>
>     if(!I2CGetAck())
>         return false;
>
>     if(!I2CStop())
>         return false;
>
>     return true;
> }
>
> Now I could nest all the if() statements, but to my mind this would
> become far more confusing than the above, especially with some of the
> longer functions.  Or perhaps set a local variable and check it before
> calling each sub function i.e.
>
> bool success;
>
> success = I2CStart();
>
> if(success)
>         success = I2CTransmit(fullAddr);
> if(success)
>         success = I2CGetAck();
>
> etc, etc...
>
> This would work and I could use one return at the end, but to my mind
> a failure at the top of the function causes unnecessary overhead in in
> checking the success variable multiple times to reach the end of the
> function. I could use a goto and a label at the end of the function,
> but that seems infinitely worse than multiple returns.
>
> Any other suggestions?
>
> Regards
>
> Mike
>
> --
> http://www.piclist.com hint: To leave the PICList
> RemoveMEpiclist-unsubscribe-requestTakeThisOuTmitvma.mit.edu
>

> bool _stdcall X9241Write(unsigned char address,
>    unsigned char instruction)
>  {
>    return
>     I2CStart() &&
>     I2CTransmit(fullAddr) &&
>     I2CGetAck() &&
>     I2CTransmit(instruction) &&
>     I2CGetAck() &&
>     I2CStop();
> }
>
> Boolean values are first class types.
>
> I really get bugged by:
>
>     if (yadayada)
>         b = true;
>     else
>         b = false;
>
> which I see in code of all sorts.
>
> What is wrong with:
>
>     b = yadayada;
>
> As to the original question re: early returns froma function:
>
> I just had an argument of this yesterday with a colleague. Personally
> I use this technique all the time and have no problem with it. It just
> becomes one more 'idiom' in my bag of tricks. When working with my own
> code know there could be early exits, so I am not stung by them. Also,
> this more closely models the way we think above solving problems.
>
> On the other hand, when you're not expecting this it can be a _big_
> pain. So I can understand those who think this is horrible style.
>
> Just an aside:
>
> This is less of problem, IMNHO, in C++ than in C. In C++ cleanup
> (destructors) still happen when you return early from a function.
>
> Bob Ammerman
> RAm Systems
> (contract development of high performance, high function, low-level
> software)
>
> --
> http://www.piclist.com hint: To leave the PICList
> RemoveMEpiclist-unsubscribe-requestKILLspammitvma.mit.edu
>

> -----Original Message-----
> From: William Chops Westfield [SMTP:spamBeGonebillwKILLspamCISCO.COM]
> Sent: Wednesday, April 11, 2001 11:14 AM
> To:   .....PICLISTspam_OUTMITVMA.MIT.EDU
> Subject:      Re: [OT]: Coding style
>
> Style opinion: put spaces after your "if"s.
>
> It's not clear to me that avoiding multiple returns in a situation like
> this
> is really that important, but if you want to avoid it, I might do somthing
> like this :
>
>         success = false;
>         while (1 /* NOT ERROR */) {
>             if (!I2CStart())
>                 break;
>
>             if (!I2CTransmit(fullAddr))
>                 break;
>
>             if (!I2CGetAck())
>                 break;
>
>             if (!I2CTransmit(instruction))
>                 break;
>
>             if (!I2CGetAck())
>                 break;
>
>             if (!I2CStop())
>                 break;
>
>             success = true;
>             break;
>         }
>         return success;
>
> A DO loop would theoretically be more appropriate, but they're uncommon
> and
> occasionally frowned upon stylistically.  Someone mentioned "break"
> shouldn't be used in while loops?  I haven't heard that one...  (I
> stripped
> your comments, but you shouldn't!)  You could make it "while (!success)",
> but that'll generate rather pointless code.  As written, there's no extra
> jump to the beginning of the loop, and a good compiler will probably
> remove
> the loop-related instructions entirely.
>
> This *is* the sort of situation where one can sometimes get away with
> GOTO.
> Neither your original code nor the above "improvement" makes it obvious
> that
> the failure returns are error conditions as clearly as something like:
>
>     if(!I2CStart())
>         goto write_error;
>
>     if(!I2CTransmit(fullAddr))
>         goto write_error;
>         :
>
> BillW
>