[SOLVED] Using a ternary operator isn't the same with if version

Projects Programming
1

Hi,

I'm working with my lcd128x64 non-blocking library, and I wanted to save several lines in some functions.

But actually it didn't run as expected.

This is my working version:

	if(y<32){
		data_buf[0] = 0x80+y;
		data_buf[1] = 0x80+x;
		data_buf[2] = pgm_read_byte_near(&img[(y*16)+(x*2)]);
		data_buf[3] = pgm_read_byte_near(&img[(y*16)+((x*2)+1)]);
	}
	
	else{
		data_buf[0] = 0x80+y-32;
		data_buf[1] = 0x88+x;
		data_buf[2] = pgm_read_byte_near(&img[(y*16)+(x*2)]);
		data_buf[3] = pgm_read_byte_near(&img[(y*16)+((x*2)+1)]);
	}

This is the new version, that didn't work:

	(y<32)	?	(data_buf[0] = 0x80+y, data_buf[1] = 0x80+x)
			:	(data_buf[1] = 0x80+y-32, data_buf[1] = 0x88+x);
	data_buf[2] = pgm_read_byte_near(&img[(y*16)+(x*2)]);
	data_buf[3] = pgm_read_byte_near(&img[(y*16)+((x*2)+1)]);

Since the element 2 and 3 are the same, so I used the conditional part with element 0 and 1.

I tested the code in desktop compiler and it worked, but maybe the arduino compiler isn't the same as desktop compiler.

2

Look again.
Carefully.

I wanted to save several lines in some functions.

Why?

3

I want to improve my code. And I thought it's a chance to use the ternary operator and also I would go from 13 lines to 4 lines.

You know when there's a block of code that can be written in a more compact way or shorter ... etc.

That's my goal basically.

4

Have you found it yet?

also I would go from 13 lines to 4 lines.

But four lines you managed to get wrong.
No gain.

I tested the code in desktop compiler and it worked,

Either the compiler was faulty, or your test methodology was.

I'm going for the latter.

5

This is my test in the desktop compiler:

int y,data_buf[4];

int main(){


    for(y=0;y<64;y++){
        (y<32)  ?   (data_buf[0] = 1, data_buf[1] = 2)
                :   (data_buf[0] = 3, data_buf[1] = 4);
        data_buf[2] = 5; data_buf[3] = 6;
        printf("%d %d %d %d #%d\n",data_buf[0], data_buf[1], data_buf[2], data_buf[3],y);
    }

    return 0;
}
6

Different code.
Like I said, look again.
Carefully

7

TheMemberFormerlyKnownAsAWOL:
Have you found it yet?

oh yes, I found it !! oh my what a mistake :slight_smile:
I uploaded the code and it's working perfectly.
But even, you don't prefer the 2nd version ? and why ?

8

I dislike the comma operator.

9

This:

		(y<32)	?	(data_buf[0] = 0x80+y, data_buf[1] = 0x80+x)
				:	(data_buf[1] = 0x80+y-32, data_buf[1] = 0x88+x);
		data_buf[2] = pgm_read_byte_near(&img[(y*16)+(x*2)]);
		data_buf[3] = pgm_read_byte_near(&img[(y*16)+((x*2)+1)]);

Should be this :

		(y<32)	?	(data_buf[0] = 0x80+y, data_buf[1] = 0x80+x)
				:	(data_buf[0] = 0x80+y-32, data_buf[1] = 0x88+x);
		data_buf[2] = pgm_read_byte_near(&img[(y*16)+(x*2)]);
		data_buf[3] = pgm_read_byte_near(&img[(y*16)+((x*2)+1)]);
10

TheMemberFormerlyKnownAsAWOL:
I dislike the comma operator.

Yes, I wasn't very interested in using this conditional operator and it's not so widely used but I thought to try it and improve my code.
But practically, is it the same as if statement ?

11

Yes, of course it is
Did you notice any difference in object code size?

12

TheMemberFormerlyKnownAsAWOL:
Did you notice any difference in object code size?

Thanks for noting me to check that.
Well, yes I save 20 bytes with using the ternary operator. So now what's the deal, is it a good approach ? especially in this part of code where I have only two parts to execute.
For readability, I think the first version is better.
For space, the ternary is I guess better.
What's your opinion ?

13

the ternary operator should be used to calculate a value. a = (b) ? c : d, not as a replacement of if

14

I thought to try it and improve my code.

Saving 20 bytes (I am surprised it saved even that) is not worth the downsides. Your code is now more difficult to read, understand, and debug, as this topic has demonstrated perfectly. It's not improved! Generally, shorter code is better, but as with so many things in life, you can take things too far. Today, you found that point, and learned something :wink:

15

wolfrose:
Thanks for noting me to check that.
Well, yes I save 20 bytes with using the ternary operator

And was there still a saving when you correctly factored the original "if" version in the same way?

16

Juraj:
the ternary operator should be used to calculate a value. a = (b) ? c : d, not as a replacement of if

Yes, in my case I have to check if (y < 64) if true execute part A else part B.
Is this ok that to execute multiple lines or it has to be like one line for part A and B ?

		(y<32)	?	(data_buf[0] = 0x80+y,	data_buf[1] = 0x80+x)    // part A
			:       (data_buf[0] = 0x80+y-32, data_buf[1] = 0x88+x); // part B
17

TheMemberFormerlyKnownAsAWOL:
And was there still a saving when you correctly factored the original "if" version in the same way?

with using the ternary operator I get 4094 bytes of program space
with if statement I get 4114.
exactly 20 bytes

18

PaulRB:
Saving 20 bytes (I am surprised it saved even that) is not worth the downsides. Your code is now more difficult to read, understand, and debug

Yes, with the if statement it's better arranged and clear.

but as with so many things in life, you can take things too far.

you mean by taking things too far is by investigating something that is rather not so useful to spend time learning the differences of using ? like investigating the use of ternary operator ?

Today, you found that point, and learned something :wink:

what point you mean exactly ? I'm still not sure of the differences between using if or ternary operator because both work the same in this case.

but as you all cleared that it's not a clever thing to do, then I'm going back to my if version:

this is the complete function:

void glcd_img(const uint8_t *img){
	if(!function_lock){
		if(!function_init_lock){x = 0; y = 0; lcd_mode = PIXEL_WRITE; function_init_lock = 1;}
		
		buf_cnts = 2; buf_cntr = 0;	lcd_cmdt = CMD;

		if(y<32){
			data_buf[0] = 0x80+y;
			data_buf[1] = 0x80+x;
			data_buf[2] = pgm_read_byte_near(&img[(y*16)+(x*2)]);
			data_buf[3] = pgm_read_byte_near(&img[(y*16)+((x*2)+1)]);
		}
		
		else{
			data_buf[0] = 0x80+y-32;
			data_buf[1] = 0x88+x;
			data_buf[2] = pgm_read_byte_near(&img[(y*16)+(x*2)]);
			data_buf[3] = pgm_read_byte_near(&img[(y*16)+((x*2)+1)]);
		}

		
		x++; if(x>7){y++;x=0;}
		if(y>64){function_init_lock = 0; system_st_flag = TASK_FINISHED;}
		function_lock = 1;
	}
	glcd_cmdt();
}
19

you mean by taking things too far is by investigating something that is rather not so useful to spend time learning the differences of using ? like investigating the use of ternary operator ?

Time spend learning and investigating is rarely wasted. Time spent making code shorter, simpler and easier to read is rarely wasted. Time spent crushing your code down so far that is becomes more complex and difficult to read is also not wasted, provided that you realise you have gone too far and back up to find that point of balance. That's what you have done, so congratulations. As experience grows you learn to recognise that point before you go past it, seeing it in your head before you type it.

Your "if" version is better than the ternary operator version, but there is still room for improvement I think. For example, what gets assigned to array indexes 2 & 3 is the same in each case, so that duplication could be removed.

20

What often gets called "the ternary operator" is simply a conditional expression.
(technically its a 'distfix' operator, "ternary operator" is unique to the C/C++ language definition
and not used elsewhere)

Use the conditional statement for statements, and the conditional expression for expressions.
Don't over-use side-effects in expressions, its confusing and poor style.

What you should have done is this, which correctly uses a conditional expression for the RHS:

 data_buf[0] = y<32 ? 0x80+y : 0x80+y-32;
 data_buf[1] = y<32 ? 0x80+x : 0x88+x;
 data_buf[2] = pgm_read_byte_near(&img[(y*16)+(x*2)]);
 data_buf[3] = pgm_read_byte_near(&img[(y*16)+((x*2)+1)]);