Searching \ for '[OT]: Coding style' in subject line. ()
Make payments with PayPal - it's fast, free and secure! Help us get a faster server
FAQ page: www.piclist.com/techref/index.htm?key=coding+style
Search entire site for: 'Coding style'.

Exact match. Not showing close matches.
PICList Thread
'[OT]: Coding style'
2001\04\11@044513 by Michael Rigby-Jones

flavicon
face
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
spam_OUTpiclist-unsubscribe-requestTakeThisOuTspammitvma.mit.edu


2001\04\11@054625 by Roland Andrag

flavicon
face
I think you want to be using a 'switch' or 'case' type statement in
conjunction with the 'break' command.. Although use of 'break' in
for/while/until type loops is arguably bad coding style, using 'break' in a
case statement is standard practise.

For example, from the Bytecraft MPC manual:

switch (ch) {
case 'a' : q = ch;
              break;   /* done if case 'a' */
case 'b' : q = 'a';  /* 'b' falls into case c */
case 'c' : p = ch;
              break;   /* done case 'c' or 'b' */
}

The switch works as follows (also from the bytecraft manual):

When a case matches, every statement following the case statement is
executed, including any statement associated with other case conditions,
until either a break statement of the end of the switch statement is
encountered.
When a break is encountered, execution ceases inside the switch and jumps to
the first statement following the switch.

Hope that helps..

Roland


{Original Message removed}

2001\04\11@055040 by Peter Tiang

flavicon
face
Err... how come your I2CStart() and I2CStop() routine
needs to generate a status ?

Unless you are doing multi-master I2C and need to
monitor for bus collision....

... otherwise, isn't it too much to return a status
for the (transmit only) I2C start and stop condition ?

Regards,
Peter Tiang

{Original Message removed}

2001\04\11@055659 by Roland Andrag

flavicon
face
Mike, apologies, just read your post again, seems you can't use a
switch/case since you are calling various functions.. Nested if/else would
be the 'nice' way of doing it then, I think, i.e.:

dummy = true;

if(!I2CStart())
        dummy = false;
else if(!I2CTransmit(fullAddr))      // send slave address
        dummy = false;
else if(!I2CGetAck())
        dummy = false;
else if(!I2CTransmit(instruction))   // send the MSB
        dummy = false;
else if(!I2CGetAck())
        dummy = false;
else if(!I2CStop())
       dummy = false;

return dummy;


Better?

Roland




{Original Message removed}

2001\04\11@061553 by Bill Westfield

face picon face
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

--
http://www.piclist.com hint: To leave the PICList
.....piclist-unsubscribe-requestKILLspamspam@spam@mitvma.mit.edu


2001\04\11@062620 by D Lloyd

flavicon
face
part 1 4390 bytes content-type:text/plain; charset=us-ascii
Hi,

From working in a team, I have found that coding styles is almost a
religous subject and caused frequent "discussions" !
"Code Complete", from the Microsoft Press, has some good pointers on coding
styles. At the end of the day, it's down to personal taste and it should be
what you feel comfortable with although things like correct indentation,
"intention" commenting and consistent use of functions do improve
'understandability.'

For example, Code Complete suggests that you do not do the following:

return_value = SomeFunction (in1, in2, out);

(where 'out' is passed as a pointer)

as there are two exit points; for consistency, it is suggested to do :

SomeFunction(in1, in2, out1, return_value)
if(return_value == xxxx)........

But this is down to the person.

Like someone else has said, I'm not sure why you are returning a value for
the start condition unless you are checking to see if the bus is still busy
prior to wading in. Some of my I2C code goes along the lines of :

uint8_t I2C_ReadTemp(void)
{

    uint8_t Temp;

    // Drive the I2C bus
    i2c_start();

    // Select device with address offset
    i2c_write(TMP_BASEADDR | TMP_ADDR_A0);

    // Select read temperature command
    i2c_write(TMP_R_READT);

    i2c_start();

    // Tell device I want to read the temperature
    i2c_write(TMP_BASEADDR | TMP_ADDR_A0 | TMP_READ);

    // Obtain value (unsigned) - NAK as only one byte wanted
    Temp=i2c_read(NAK);

    i2c_stop();

    return(Temp);

}

Please critcise/destroy as necessary(!)

As to if elses/switches, it is always worth checking the list file to see
what the compiler has been up to in order to determine efficiency.

My tuppence worth, anyway

Dan



(Embedded     Michael Rigby-Jones <mrjonesspamKILLspamNORTELNETWORKS.COM>spamKILLspamMITVMA.MIT.EDU>> image moved   11/04/2001 09:41
to file:
pic15718.pcx)





Please respond to pic microcontroller discussion list
     <
.....PICLISTKILLspamspam.....MITVMA.MIT.EDU>
Sent by:  pic microcontroller discussion list <EraseMEPICLISTspam_OUTspamTakeThisOuTMITVMA.MIT.EDU>


To:   PICLISTspamspam_OUTMITVMA.MIT.EDU
cc:
Subject:  [OT]: Coding style

Security Level:?         Internal


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
@spam@piclist-unsubscribe-requestKILLspamspammitvma.mit.edu






part 2 165 bytes content-type:application/octet-stream; (decode)

part 3 105 bytes
--
http://www.piclist.com hint: To leave the PICList
KILLspampiclist-unsubscribe-requestKILLspamspammitvma.mit.edu


2001\04\11@064113 by ISO-8859-1?Q?Ruben_J=F6nsson?=

flavicon
face
bool _stdcall X9241Write(unsigned char address,
 unsigned char instruction)
{
 if (
  I2CStart() &&
  I2CTransmit(fullAddr) &&
  I2CGetAck() &&
  I2CTransmit(instruction) &&
  I2CGetAck() &&
  I2CStop()
 )
  return TRUE;
 else
  return FALSE;
}


Date sent:              Wed, 11 Apr 2001 09:41:44 +0100
Send reply to:          pic microcontroller discussion list <RemoveMEPICLISTTakeThisOuTspamMITVMA.MIT.EDU>
From:                   Michael Rigby-Jones <spamBeGonemrjonesspamBeGonespamNORTELNETWORKS.COM>
Subject:                [OT]: Coding style
To:                     TakeThisOuTPICLISTEraseMEspamspam_OUTMITVMA.MIT.EDU

{Quote hidden}

==============================
Ruben Jvnsson
AB Liros Elektronik
Box 9124, 200 39 Malmv, Sweden
TEL INT +46 40142078
FAX INT +46 40947388
rubenEraseMEspam.....pp.sbbs.se
==============================

--
http://www.piclist.com hint: To leave the PICList
EraseMEpiclist-unsubscribe-requestspammitvma.mit.edu


2001\04\11@074442 by Bob Ammerman

picon face
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-requestEraseMEspamEraseMEmitvma.mit.edu


2001\04\11@095614 by rubenj

flavicon
face
Is it guaranteed that this statement evaluates from left to right and
evaluation stops as soon as there is a false returned from one of the
functions in the statements?


Date sent:              Wed, 11 Apr 2001 07:38:25 -0400
Send reply to:          pic microcontroller discussion list <RemoveMEPICLISTspam_OUTspamKILLspamMITVMA.MIT.EDU>
From:                   Bob Ammerman <RemoveMERAMMERMANTakeThisOuTspamspamPRODIGY.NET>
Subject:                Re: [OT]: Coding style
To:                     EraseMEPICLISTspamspamspamBeGoneMITVMA.MIT.EDU

{Quote hidden}

==============================
Ruben Jvnsson
AB Liros Elektronik
Box 9124, 200 39 Malmv, Sweden
TEL INT +46 40142078
FAX INT +46 40947388
rubenSTOPspamspamspam_OUTpp.sbbs.se
==============================

--
http://www.piclist.com hint: To leave the PICList
spamBeGonepiclist-unsubscribe-requestSTOPspamspamEraseMEmitvma.mit.edu


2001\04\11@120145 by Bill Westfield

face picon face
     if (
      I2CStart() &&
      I2CTransmit(fullAddr) &&
      I2CGetAck() &&
      I2CTransmit(instruction) &&
      I2CGetAck() &&
      I2CStop()
     )

Is C *REQUIRED* to EXECUTE such expressions in order and short-circuit the
evaluation as soon as possible?  If so, then of course it could be:

       return(
              I2CStart() &&
              I2CTransmit(fullAddr) &&
              I2CGetAck() &&
              I2CTransmit(instruction) &&
              I2CGetAck() &&
              I2CStop()
             )

BillW

--
http://www.piclist.com hint: To leave the PICList
KILLspampiclist-unsubscribe-requestspamBeGonespammitvma.mit.edu


2001\04\12@110127 by Michael Rigby-Jones

flavicon
face
> -----Original Message-----
> From: Bob Ammerman [SMTP:EraseMERAMMERMANspamEraseMEPRODIGY.NET]
> Sent: Wednesday, April 11, 2001 12:38 PM
> To:   @spam@PICLIST@spam@spamspam_OUTMITVMA.MIT.EDU
> Subject:      Re: [OT]: Coding style
>
> bool _stdcall X9241Write(unsigned char address,
>    unsigned char instruction)
>  {
>    return
>     I2CStart() &&
>     I2CTransmit(fullAddr) &&
>     I2CGetAck() &&
>     I2CTransmit(instruction) &&
>     I2CGetAck() &&
>     I2CStop();
> }
>
This is very compact and not a goto or multiple return in sight.  I'm
presuming the compiler will compile this such that any one function
returning a fail will immediately cause the function to return false without
calling any further functions.  Is this a reasonable assumption, and will it
hold true for any (Ansi) C compiler?

Mike

--
http://www.piclist.com hint: PICList Posts must start with ONE topic:
[PIC]:,[SX]:,[AVR]: ->uP ONLY! [EE]:,[OT]: ->Other [BUY]:,[AD]: ->Ads


2001\04\12@110139 by Michael Rigby-Jones

flavicon
face
{Quote hidden}

The initial version of this driver used goto's to a common exit point which
I considered a reasonable use of a goto.  However, a colleague made some
disparaging comments about it and made me think twice about it!  I think
that Code Complete mentions that break's within loops can make for difficult
reading, but also says that, as with almost anything, there are exceptions
to the rule.  The above seems reasonable.

Thanks for the suggestions.

Mike

--
http://www.piclist.com hint: PICList Posts must start with ONE topic:
[PIC]:,[SX]:,[AVR]: ->uP ONLY! [EE]:,[OT]: ->Other [BUY]:,[AD]: ->Ads


2001\04\12@110149 by Michael Rigby-Jones

flavicon
face
> -----Original Message-----
> From: Peter Tiang [SMTP:TakeThisOuTpetertiang.....spamTakeThisOuTPD.JARING.MY]
> Sent: Wednesday, April 11, 2001 10:52 AM
> To:   TakeThisOuTPICLISTKILLspamspamspamMITVMA.MIT.EDU
> Subject:      Re: [OT]: Coding style
>
> Err... how come your I2CStart() and I2CStop() routine
> needs to generate a status ?
>
> Unless you are doing multi-master I2C and need to
> monitor for bus collision....
>
> ... otherwise, isn't it too much to return a status
> for the (transmit only) I2C start and stop condition ?
>
> Regards,
> Peter Tiang
>
All the functions call the functions bool SetSDA(bool state) which not only
sets the bus line to the required state, it also polls the read back line
until either the bus line has reached the required state, or it has timed
out.  This could be due to hung slave, faulty hardware etc.  If I performed
a start condition and both lines were stuck low for instance, the function
would be aborted imediately rather than chugging away through the rest of
the code trying to send or receive data.

My goal is to make the drivers (which are compiled as a Win32 DLL) as robust
as possible, with decent error reporting.  The driver operates perfectly,
but as this driver is likely to be shipped along with some hardware to
customers, I want to make the code as maintainable as possible.  If this was
a quick one-off for myself, or for in house use I wouldn't give this a
second thought, but I feel compelled to make the best job of this that I
can.

Regards

Mike

--
http://www.piclist.com hint: PICList Posts must start with ONE topic:
[PIC]:,[SX]:,[AVR]: ->uP ONLY! [EE]:,[OT]: ->Other [BUY]:,[AD]: ->Ads


2001\04\12@112640 by Bob Ammerman

picon face
Yes, this your assumptions are correct, and this behaviour is required by
the ANSI standard.

Bob Ammerman
RAm Systems
(contract development of high performance, high function, low-level
software)

----- Original Message -----
From: "Michael Rigby-Jones" <.....mrjonesspamRemoveMENORTELNETWORKS.COM>
To: <RemoveMEPICLISTspamspamBeGoneMITVMA.MIT.EDU>
Sent: Thursday, April 12, 2001 9:03 AM
Subject: Re: [OT]: Coding style


> > {Original Message removed}

2001\04\12@125635 by Bill Westfield

face picon face
>>  I'm presuming the compiler will compile this such that any one function
>> returning a fail will immediately cause the function to return false
>> without calling any further functions.  Is this a reasonable assumption,
>> and will it hold true for any (Ansi) C compiler?
>
> Yes, this your assumptions are correct, and this behaviour is required by
> the ANSI standard.

Microntroller C's tend to drift away from "full ansi compliance."  Does
anyone know for sure if any of the popular PIC C's WON'T handle the
short-cutting of the boolean expression correctly?

BillW

--
http://www.piclist.com hint: PICList Posts must start with ONE topic:
[PIC]:,[SX]:,[AVR]: ->uP ONLY! [EE]:,[OT]: ->Other [BUY]:,[AD]: ->Ads


2001\04\12@143921 by Dale Botkin

flavicon
face
On Thu, 12 Apr 2001, William Chops Westfield wrote:

> >>  I'm presuming the compiler will compile this such that any one function
> >> returning a fail will immediately cause the function to return false
> >> without calling any further functions.  Is this a reasonable assumption,
> >> and will it hold true for any (Ansi) C compiler?
> >
> > Yes, this your assumptions are correct, and this behaviour is required by
> > the ANSI standard.
>
> Microntroller C's tend to drift away from "full ansi compliance."  Does
> anyone know for sure if any of the popular PIC C's WON'T handle the
> short-cutting of the boolean expression correctly?

I've looked at the output of CCS PCM under similar circumstances, and it
does quit testing as soon as it can.

Dale
---
The most exciting phrase to hear in science, the one that heralds new
discoveries, is not "Eureka!" (I found it!) but "That's funny ..."
               -- Isaac Asimov

--
http://www.piclist.com hint: PICList Posts must start with ONE topic:
[PIC]:,[SX]:,[AVR]: ->uP ONLY! [EE]:,[OT]: ->Other [BUY]:,[AD]: ->Ads


2001\04\12@145239 by Mike Mansheim

flavicon
face
----- Forwarded by Michael J Mansheim/Graco on 04/12/01 01:45 PM -----

>> Yes, this your assumptions are correct, and this behaviour is required
by
>> the ANSI standard.

> Microntroller C's tend to drift away from "full ansi compliance."  Does
> anyone know for sure if any of the popular PIC C's WON'T handle the
> short-cutting of the boolean expression correctly?

There seemed to be a bit of interest in this question, so I thought I would
test it.
With CCS, it works as Bob says it should.

Here's the code if anyone wants to be sure I tested correctly:

To check different values of F1 & F2, I just changed them and re-compiled.
I
used a breakpoint to see if function2() was entered.
With F1 = 0 and F2 = 1, result = 0 and function2() was not entered -
exactly
the desired behavior!
In case anyone is wondering about testing globals in the test functions
rather
than passing arguments (which would be cleaner), I did it this way so the
test
would look more like the original question (i.e. functions called that do
not
take arguments).
By the way, from an earlier discussion, ints are 8 bits in the CCS
compiler.

unsigned int F1, F2;

unsigned int test(void);
unsigned int function1(void);
unsigned int function2(void);

void main(void)
{
    unsigned int result;

    F1 = 0;
    F2 = 1;

    result = test();
}

unsigned int test(void)
{
    return function1() && function2();
}

unsigned int function1(void)
{
    unsigned int return_value;

    if (F1 == 1) return_value = 1;
    else       return_value = 0;

    return return_value;
}

unsigned int function2(void)
{
    unsigned int return_value;

    if (F2 == 1) return_value = 1;
    else       return_value = 0;

    return return_value;
}
Just for yuks, I tried the original method, where test() would look like:

unsigned int test(void)
{
    if (!function1()) return 0;
    if (!function2()) return 0;
    return 1;
}

because this looked fine to me, and is probably how I would have done
it.  Not being a C guru, I didn't know you could do what Bob posted.
Not surprisingly, this compiles larger.  So the 'cleaner' look also
compiles more efficiently - not always the case with C.

--
http://www.piclist.com hint: PICList Posts must start with ONE topic:
[PIC]:,[SX]:,[AVR]: ->uP ONLY! [EE]:,[OT]: ->Other [BUY]:,[AD]: ->Ads


More... (looser matching)
- Last day of these posts
- In 2001 , 2002 only
- Today
- New search...