Code Style I DO NOT Like!

Go To Last Post
31 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Been working through some Microchip TechBriefs. In TB3217 (introduction to TCA in Mega0, Tinyo/1, and DA chips) came across this line of code and several like it:

 

	TCA0.SPLIT.CTRLA = TCA_SPLIT_CLKSEL_DIV16_gc    /* set clock source (sys_clk/16) */
		| TCA_SPLIT_ENABLE_bm;		/* start timer */	

 

Now, I have relatively little doubt that it is legal. But, this is the first time I have encountered code like this and I do not like it. I understand why it is used, but that does not make it "good". It is so easy to loose where the true end of the code line is! 

 

Might as well write:

 

TCA0.SPLIT.CTRLA = /* what is this? */ TCA_SPLIT_CLKSEL_DIV16_gc | TCA_SPLIT_ENABLE_bm;

Why not? It seems to be allowed and that must make it "A Good Thing".

 

[edit] Typo correct

 

Jim

 

Until Black Lives Matter, we do not have "All Lives Matter"!

 

 

Last Edited: Thu. Oct 21, 2021 - 06:14 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

They are trying to micromanage comments...just  group the comments at the end of the line and all will be well.   

Either they think you can't figure out which comment goes with which TCA_ item, or it is brewed up by some automatic bit blender/generator without regard to common sense.

 

The only good thing is they aren't wasting even more valuable lines placing the comments elsewhere.    Then you end up with very tall and narrow code that is inefficiently displayed.

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
TCA0.SPLIT.CTRLA = TCA_SPLIT_CLKSEL_DIV16_gc | TCA_SPLIT_ENABLE_bm;//set clk src sysclk/16, start timer

 

One liner, comment at end, anything more is Ridiculous. Readability in code matters. Furthermore a bunch of .H includes with nothing but a single line of code in them is equally Ridiculous.

Last Edited: Thu. Oct 21, 2021 - 05:26 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Assembler is more restricted on multi-lines, yet the backslash "\" at the EOL does helps.

ASM designer may use this also in long tables.

 

 

ldi     r16, (1 << INT0) |  /* see sqirr */ \
        (1 << PCIE)    /* see bT13PSlave */

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Assembler is more restricted on multi-lines

Well, you would likely just do:

ldi r16, (1 << INT0) | (1 << PCIE) ; see sqirr, bT13PSlave or ask Larry J about this

Rarely would you need multilines in asm (but never say never)

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 2

Hey: I use that code style quite often.

 

Imagine if you were configuring that register with settings for all 8 bits, that line would become extraordinarily long and require horizontal scrolling to read it.

Plus; by giving each setting its own line you can also add a comment for that particular setting.should you wish.

 

BTW: The single line thing becomes completely out of hand on 16 and 32-bit processors.

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 2

I personally like the style A LOT but I fear the indentation is wrong. It was surely meant to be:

    TCA0.SPLIT.CTRLA = TCA_SPLIT_CLKSEL_DIV16_gc    /* set clock source (sys_clk/16) */
                     | TCA_SPLIT_ENABLE_bm	    /* start timer */
                     | TCA_SPLIT_SOMETHING_ELSE_bm  /* and something else */
                     | TCA_SPLIT_AND_AGAIN_bm;      /* etc etc. */

It allows you to comment on the action of each bit or group of bits that is being set.

 

If you have one line and a combined comment:

    TCA0.SPLIT.CTRLA = TCA_SPLIT_CLKSEL_DIV16_gc | TCA_SPLIT_ENABLE_bm | TCA_SPLIT_SOMETHING_ELSE_bm | TCA_SPLIT_AND_AGAIN_bm; /* set clock source (sys_clk/16), start timer, and something else,  etc etc. */

Then not only (with these "long symbols") can you end up with stupidly long lines but you don't know which bit is responsible for which setting mentioned in the comment. Also that simply looks like a "lot of noise" to me - very difficult to see the individual detail while the multi-line thing lets you see everything.

 

You might consider the same for function interfaces too:

int                      /* error code 0=OK, 1=not possible                */
UART_init(
    struct UART * pUART, /* a UART instance such as &UARTD or &UARTE       */
    int baud,            /* baud rate such as 300, 9600, 19200 etc.        */
    bool parity,         /* set for parity false=no (8bit) true=yes (7bit) */
    int bits,            /* data bits can be 5..98, 7 or 8 most usual      */
    etc.
) {
    // stuff
}

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ka7ehk wrote:
It is so easy to loose where the true end of the code line is! 

the layout is particularly poor - it would be a lot clearer if they'd lined it up nicely, like this:

TCA0.SPLIT.CTRLA = TCA_SPLIT_CLKSEL_DIV16_gc    /* set clock source (sys_clk/16) */
		 | TCA_SPLIT_ENABLE_bm;		/* start timer                      */

 

Personally, I'd write it as:

TCA0.SPLIT.CTRLA = TCA_SPLIT_CLKSEL_DIV16_gc |    /* set clock source (sys_clk/16) */
		   TCA_SPLIT_ENABLE_bm;		  /* start timer                      */

so the trailing 'OR' operator on the 1st line tells you that it can't be the end of the statement

 

Or even:

TCA0.SPLIT.CTRLA = TCA_SPLIT_CLKSEL_DIV16_gc |    /* set clock source (sys_clk/16) */
		   TCA_SPLIT_ENABLE_bm       ; 	  /* start timer                      */

Which makes the end-of-statement semicolon stand out.

 

But there is certainly no problem with comments within a statement: a comment is just "whitespace" as far as the compiler is concerned - so a comment can go anywhere that whitespace is allowed

 

Taking it to the extreme, you could have:

TCA0.                                            /* timer-counter peripheral 0  */
    SPLIT.                                       /* in "split" mode             */
          CTRLA =                                /* Control register A          */
                   TCA_SPLIT_CLKSEL_DIV16_gc |   /* clock source = (sys_clk/16) */
		   TCA_SPLIT_ENABLE_bm       ;   /* start timer                    */

though I think that's taking it a bit far!  surprise

 

EDIT

 

Cliff beat me to it.

again.

 

So I agree with the previous 2: I find it is an excellent style - especially where you have long names, and are setting lots of bits/fields.

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
Last Edited: Thu. Oct 21, 2021 - 08:42 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

BTW all of the above is somewhat related to the points I was also making in this post: https://www.avrfreaks.net/commen... too. In fact look at the ringuffer code from Dean in LUFA

 

https://github.com/abcminiuser/l...

 

I think it's close to a perfect example of how code should be laid out and documented (which is true for so much of what Dean has written !)

Last Edited: Thu. Oct 21, 2021 - 08:59 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

avrcandies wrote:
or ask Larry J about this

 

I tried to search for Larry J, but there are many, so I do not know which one is right.

You are kindly invited to explain what it means, or to remove this comment, please.

 

Edit: Hold on, it is not important what you will say.

Last Edited: Thu. Oct 21, 2021 - 09:11 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
I think it's close to a perfect example of how code should be laid out and documented 

Certainly looks pretty good.

 

yes

 

But I find these comments a bit of a mess:

		typedef struct
		{
			uint8_t* In; /**< Current storage location in the circular buffer. */
			uint8_t* Out; /**< Current retrieval location in the circular buffer. */
			uint8_t* Start; /**< Pointer to the start of the buffer's underlying storage array. */
			uint8_t* End; /**< Pointer to the end of the buffer's underlying storage array. */
			uint16_t Size; /**< Size of the buffer's underlying storage array. */
			uint16_t Count; /**< Number of bytes currently stored in the buffer. */
		} RingBuffer_t;

 

I'd prefer those comments to be nicely aligned:

		typedef struct
		{
			uint8_t* In;    /**< Current storage location in the circular buffer. */
			uint8_t* Out;   /**< Current retrieval location in the circular buffer. */
			uint8_t* Start; /**< Pointer to the start of the buffer's underlying storage array. */
			uint8_t* End;   /**< Pointer to the end of the buffer's underlying storage array. */
			uint16_t Size;  /**< Size of the buffer's underlying storage array. */
			uint16_t Count; /**< Number of bytes currently stored in the buffer. */
		} RingBuffer_t;

makes it much easier to see the code from the comments, IMO

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

 

I tend to agree but the idea is not that you read those comments in the code but that you read them here:

 

http://www.fourwalledcubicle.com...

 

 

(Can't say I'm a fan of the element types being left aligned under "Data fields" though!)

 

EDIT: also I think I would have preferred the struct elements listed in their defined order, not alpha-sorted.

Last Edited: Thu. Oct 21, 2021 - 09:10 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
Can't say I'm a fan of the element types being left aligned

Yeah - that's weird! surprise

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

As with everything in life.   It depends on the circumstances.

 

The one line per SFR bit is suitable for a Tutorial or an Application Note.   Or possibly for an automated Project Generator.

 

But for your own project comments / reminders it just irritates (me).

I am happier with a single line assignment statement.   Preferably with a terse explanation.

 

However,  different people have different styles and preferences.   I like to adopt something that is the right balance between readability and information.

Too many lines and my eyes glaze over.  Superfluous comments irritate.   SnowWhite just needs to find the right porridge bowl.

 

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Doxygen is so extensive I'm sure their are options for all these things. Actually the major stumbling block to Doxygen'ing code is getting the code authors up to speed about how to use Doxygen. Thankfully things like the options that are set in the Doxyfile to configure the whole look of the documentation is something probably done by the project architect and not something mere engineers need concern themselves with. (though I would probably be pushing a change to code review to see if I could persuade them that a different Doxyfile setting to change indent or ordering might be accepted by the author/code reviewers!). But often even this kind of "style" thing comes down to personal preference.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

david.prentice wrote:
But for your own project comments / reminders it just irritates (me).
Depends if you are working in a team or for open publication rather than alone. Comments should be there to explain to others your method/logic, not necessarily for your own use (you may well remember?) though it's true that, with age, any trail of breadcrumbs you can leave for yourself may come in useful when you return to code after several years.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

david.prentice wrote:
I am happier with a single line assignment statement.  

but, as noted, with names like "TCA0.SPLIT.CTRLA" and "TCA_SPLIT_CLKSEL_DIV16_gc" and  "TCA_SPLIT_ENABLE_bm" it very soon becomes unwieldy to have them all on a single line - so you have to have some strategy for how to split lines.

 

If the line's going to be split at all, I think it makes sense to make it "one item per line" (unless there is some logical grouping). Whether you then choose to comment each line is a separate question.

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I am happier with a single line assignment statement.   Preferably with a terse explanation.

Sometimes all you need is to say: config ADC @10KHz, autosampling on ch 5

Oher times you need almost a bit-by-bit playback, so one size doesn't fit all.

Don't overload your screen with things scrunched in, but don't be grossly wasteful either, or you'll spend your life scrolling all around, or using a small font (with eyestrain) to get a good overview of what is going on.

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Now that it has been mentioned, I did see that style used in FatFS for function argument lists.

 

Did not like it there, either, but it was better formatted and more readable. I could learn to like #7 and #8.

 

Jim

 

Until Black Lives Matter, we do not have "All Lives Matter"!

 

 

Last Edited: Thu. Oct 21, 2021 - 03:54 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The formatting problems I have seen are from tabs. Everyone converts their tabs to spaces when they look at their source with a different editor.

 

And a dumb meme for the win:

 

https://insanelab.com/blog/notes/spaces-vs-tabs/

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

ron_sutherland wrote:

The formatting problems I have seen are from tabs. Everyone converts their tabs to spaces when they look at their source with a different editor.

 

And a dumb meme for the win:

 

https://insanelab.com/blog/notes/spaces-vs-tabs/

our coding standard mandates that all code is written with editors configured to insert spaces for tabs at 4 character intervals. It works well, we each see others code laid out exactly as written and not playing some lottery as to whether tabs are set similarly. I usually use editors set to view whitespace so either see dots or the odd rogue arrow which is quickly expunged. One of the gates for or revision control system bars any code with trailing spaces too! 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I don't like cryptic names that require a comment to explain.  If the cryptic names are spelled out instead, very few comments are needed.  That makes the code much easier to read.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

It was surely meant to be:

    TCA0.SPLIT.CTRLA = TCA_SPLIT_CLKSEL_DIV16_gc    /* set clock source (sys_clk/16) */
                     | TCA_SPLIT_ENABLE_bm	    /* start timer */
                     | TCA_SPLIT_SOMETHING_ELSE_bm  /* and something else */
                     | TCA_SPLIT_AND_AGAIN_bm;      /* etc etc. */

 

I prefer to see operators at the end of the line being continued, which makes it more obvious that there is more coming, as opposed to "I forgot a semicolon."

    TCA0.SPLIT.CTRLA = TCA_SPLIT_CLKSEL_DIV16_gc |    /* set clock source (sys_clk/16) */
                       TCA_SPLIT_ENABLE_bm |          /* start timer */
                       TCA_SPLIT_SOMETHING_ELSE_bm |  /* and something else */
                       TCA_SPLIT_AND_AGAIN_bm;        /* etc etc. */

 

Jim: what format DO you prefer when a statement becomes too long to fit on a reasonable-sized single line?

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

With regard to coding style, I have only one beef with LUFA:

Many of the lines are just too long.

The ring buffer example has 67 lines with more than 80

characters (counting tabs as 4), 25 of them at least 100.

That might be OK if one never reads the code.

I do not have the monster monitor a lot of other people seem to have.

Even if I did, printing would still be a pain.

Doxygen is for documentation.

It does not help one read the actual code.

 

That said, LUFA is something I expect I could not have written.

Dean did.

Moderation in all things. -- ancient proverb

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I stack descriptive comment lines before the code line being executed.

 

Jim

 

Until Black Lives Matter, we do not have "All Lives Matter"!

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I stack descriptive comment lines before the code line being executed.

I like stacking ahead when there is something of significance or complexity to explain. That is, when a side comment would not suffice, or more details are needed that require space to do so.

 

Too many times we see a lot of this:

 

//Configure PORTA for outputs

DDRA= 0xFF;

 

// The following line of code turns on the Pump LED indicator

PORTA |= (1<<PumpLED);

 

If going to burn up a screen line, give some important,  non-obvious info or explanation

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

westfw wrote:
I prefer to see operators at the end of the line being continued, which makes it more obvious that there is more coming

likewise - see #8

 

although I would line them up:

    TCA0.SPLIT.CTRLA = TCA_SPLIT_CLKSEL_DIV16_gc   |  /* set clock source (sys_clk/16) */
                       TCA_SPLIT_ENABLE_bm         |  /* start timer */
                       TCA_SPLIT_SOMETHING_ELSE_bm |  /* and something else */
                       TCA_SPLIT_AND_AGAIN_bm      ;  /* etc etc. */

which, I think, highlights it.

 

 

Jim: what format DO you prefer when a statement becomes too long to fit on a reasonable-sized single line?

good question!

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

A modern editor with syntax highlighting makes things a bit easier methinks.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0


IntelliSense is also quite helpful; it removes much of the grammar correction spin loop I go through. I am trying Netbeans in MPLAB now but plan to do my edits with that thing that has IntelliSense. If a mouse over the variable pulls up the comment next to it from the header, then duplicating those is surely not needed.

 

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

For the initial author, perhaps. But now you are assuming that (a) the intellisense editor is available to all future code viewers, and (b) that they choose to use it. Perhaps they prefer a plain text editor?

 

Neil

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

 

IntelliSense is more like a grammar checker. Make all the red squigglies go away before compiling.

 

 

The mouse-over trick may not be IntelliSense, but I guess most editors don't even do that. I can cut and paste those comments if the boss wants.

 

 

Last Edited: Fri. Oct 22, 2021 - 04:42 PM