Skip to content

Small code style change - #8231

Closed
cpsw wants to merge 1 commit into
RIOT-OS:masterfrom
cpsw:patch-1
Closed

Small code style change#8231
cpsw wants to merge 1 commit into
RIOT-OS:masterfrom
cpsw:patch-1

Conversation

@cpsw

@cpsw cpsw commented Dec 9, 2017

Copy link
Copy Markdown

A code style suggestion.

A code style suggestion.
@miri64 miri64 added Area: core Area: RIOT kernel. Handle PRs marked with this with care! Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Dec 9, 2017
@miri64 miri64 self-assigned this Dec 9, 2017
@miri64

miri64 commented Dec 9, 2017

Copy link
Copy Markdown
Member

Welcome to RIOT @cpsw. Please prefix the commit with the module you are changing. Also, can you please provide a justification for your PR?

@smlng

smlng commented Dec 11, 2017

Copy link
Copy Markdown
Member

We appreciate contributions on enhancing RIOTs code in style, readability and documentation, but the proposed changes are (IMHO) rather subjective to personal coding style. In this case the ternary operator is perfectly valid and used properly. In general: using the ternary operator is not against RIOTs coding conventions. We had discussion on that several times and - for sure - there are places where the ternary operator is not appropriate or falsely applied, however, this is not the case here.

I'm against these changes, otherwise we'll likely have lots of such changes back and forth.

@miri64

miri64 commented Dec 11, 2017

Copy link
Copy Markdown
Member

Hi @smlng, in general, I agree with you, but I also would like @cpsw defend their contribution.

@smlng

smlng commented Dec 11, 2017

Copy link
Copy Markdown
Member

Sure! No need to rush, just wanted to raise my concerns.

@kaspar030

Copy link
Copy Markdown
Contributor

In general: using the ternary operator is not against RIOTs coding conventions. We had discussion on that several times and - for sure - there are places where the ternary operator is not appropriate or falsely applied, however, this is not the case here.

+1

@cpsw

cpsw commented Jan 20, 2018

Copy link
Copy Markdown
Author

Hi @miri64, thanks! I'm a professor, teaching programming for the last 5 years. I'm concerned with code understanding. I know that if ternary is something really common in practice. However, for beginners, someone that has never seen that, it requires an additional knowledge to understanding to code. Furthermore, in my opinion, it is clearer and faster to see the alternatives when we have an if/else statement, as I suggest.

@kaspar030, can you please point me to places where if ternary operator is not appropriate or falsely applied? Thanks

@miri64

miri64 commented Jan 30, 2018

Copy link
Copy Markdown
Member

@kaspar030, can you please point me to places where if ternary operator is not appropriate or falsely applied? Thanks

@cpsw it was never merged, but see e.g. this discussion: #7276 (comment)

@miri64 miri64 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really not in favor of merging something that is just a change of style. But maybe some other contributor can be convinced.

@miri64 miri64 removed their assignment Mar 15, 2018
@kaspar030

Copy link
Copy Markdown
Contributor

Ternary operators are fine. Don't take the one precious little syntactically hygenic line-saver from us poor low-level C coders! ;)

Closing as wontfix.

@kaspar030 kaspar030 closed this Mar 27, 2018
@miri64 miri64 added the State: won't fix State: The issue can not or will not be fixed label Mar 27, 2018
Comment thread core/clist.c
psize = 0;
for (i = 0; i < insize; i++) {
psize++;
q = (q->next == oldhead) ? NULL : q->next;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to make them multi-line now a days:

q = (q->next == oldhead)
        ? NULL
        : q->next;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mhm nice idea to keep the 80 chars limit ... in same cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: core Area: RIOT kernel. Handle PRs marked with this with care! State: won't fix State: The issue can not or will not be fixed Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants