Skip to content

Small code style change#8231

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

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

Conversation

@cpsw
Copy link

@cpsw cpsw commented Dec 9, 2017

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
Copy link
Member

miri64 commented Dec 9, 2017

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
Copy link
Member

smlng commented Dec 11, 2017

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
Copy link
Member

miri64 commented Dec 11, 2017

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

@smlng
Copy link
Member

smlng commented Dec 11, 2017

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

@kaspar030
Copy link
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
Copy link
Author

cpsw commented Jan 20, 2018

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
Copy link
Member

miri64 commented Jan 30, 2018

@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)

Copy link
Member

@miri64 miri64 left a comment

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
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
psize = 0;
for (i = 0; i < insize; i++) {
psize++;
q = (q->next == oldhead) ? NULL : q->next;
Copy link
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
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