4 ms·

I wouldn't even write this way because the else confuses what's happening. void fn(int a, int b) { string ret = "not"; if (a == b)

I wouldn't even write this way because the else confuses what's happening. void fn(int a, int b) { string ret = "not"; if (a == b) ret = "equal"; printf(ret); } *I understand, this is not a good optimization in every language.

ummonk 5y ago
Your code is far less clear to me than the above code. Though I'd probably write: void fn(int a, int b) { printf(a == b ? "equal" : "not"); }
throwawaylinux 5y ago
How does the 'else' confuse what is happening?
- Supermancho 5y ago
  The function will perform a printf, so nesting it in an if-else subconsciously implies it's optional. This is a quirk of human text comprehension. In a small program, it seems arbitrary because you can see it all at once. Make the branches do more than simply printf and you can see why you want to frontload the else, rather than pick through the if/else to see what always happens.
  - throwawaylinux 5y ago
    I can kind of see what you mean, but if you make branches do more then it's also quite possible the common actions become more tedious to factor into common code. And I don't think it's good practice to read control statements like that (obviously indented loops can't be assumed optional either). int pow2(int v, int pow) { int result; if (pow >= 0) result = v << pow; else result = v >> -pow; return result; } Looking at this, both sides of the if/else perform a multiply by 2^pow, you can't take the operation to possibly not happen. So I agree factoring common operations can be a good thing, but I think it's really a case by case basis and my code is fine. I would also caution against assuming something may not happen elsewhere in a function just because you see it inside a conditional block. This happens frequently to various degrees and I've never encountered a style policy preventing it or any compiler or linter warnings for it.
nybble41 5y ago
IMHO the original version without the mutable local variable was much more obvious. In any language. Though I would also accept: void fn(int const a, int const b) { printf(a == b ? "equal" : "not"); } to avoid duplicating the call to printf(). If this were Rust I might suggest: fn theFn(a: isize, b: isize) { let ret; if a == b { ret = "equal"; } else { ret = "not"; } println!("{}", ret); } but only because Rust, unlike C or C++, will enforce that `ret` is assigned exactly once before it is used.
- Supermancho 5y ago
  > IMHO the original version without the mutable local variable was much more obvious. In any language. This is not about obviousness. You can write the same expression lots of different ways, which are all obvious. This is still a question about testable paths and code hygiene. Nesting the same function call in both branches of if/else unintentionally obscures that it's not a conditional printf call. If you're going to write: printf(a == b ? "equal" : "not"); you don't need the function at all, completely missing the point. Your second example is definitely better in Rust.
  - nybble41 5y ago
    > Nesting the same function call in both branches of if/else unintentionally obscures that it's not a conditional printf call. And the version with mutation masks the fact that the initial value assigned to `ret` may or may not be used, which seems like a very similar problem to me. Plus it involves mutation, which IMHO significantly increases the complexity of any code. Unfortunately C offers limited options here; as cryptic and controversial as it may be, the ternary operator is the only (non-hackish) way to make an expression which selects between two or more values based on a boolean condition without either the possibility of failing to assign a value or requiring a "default" value which at least some paths through the function won't use. (What if there is no reasonable default value and all the required branches are expensive to compute?) > If you're going to write: … printf(a == b ? "equal" : "not"); … you don't need the function at all, completely missing the point. The function still encapsulates the specific comparison and the way the result is reported—exactly like the other versions—so I wouldn't say it's completely useless despite containing only one line of code. This is, of course, merely a toy example. You could assign the string to a named const variable if you wanted to make it slightly more self-documenting, though I question whether that would make this particular code any more readable.

ummonk 5y ago

Your code is far less clear to me than the above code. Though I'd probably write: void fn(int a, int b) { printf(a == b ? "equal" : "not"); }

throwawaylinux 5y ago

How does the 'else' confuse what is happening?

Supermancho 5y ago
The function will perform a printf, so nesting it in an if-else subconsciously implies it's optional. This is a quirk of human text comprehension. In a small program, it seems arbitrary because you can see it all at once. Make the branches do more than simply printf and you can see why you want to frontload the else, rather than pick through the if/else to see what always happens.
- throwawaylinux 5y ago
  I can kind of see what you mean, but if you make branches do more then it's also quite possible the common actions become more tedious to factor into common code. And I don't think it's good practice to read control statements like that (obviously indented loops can't be assumed optional either). int pow2(int v, int pow) { int result; if (pow >= 0) result = v << pow; else result = v >> -pow; return result; } Looking at this, both sides of the if/else perform a multiply by 2^pow, you can't take the operation to possibly not happen. So I agree factoring common operations can be a good thing, but I think it's really a case by case basis and my code is fine. I would also caution against assuming something may not happen elsewhere in a function just because you see it inside a conditional block. This happens frequently to various degrees and I've never encountered a style policy preventing it or any compiler or linter warnings for it.

nybble41 5y ago

IMHO the original version without the mutable local variable was much more obvious. In any language. Though I would also accept: void fn(int const a, int const b) { printf(a == b ? "equal" : "not"); } to avoid duplicating the call to printf(). If this were Rust I might suggest: fn theFn(a: isize, b: isize) { let ret; if a == b { ret = "equal"; } else { ret = "not"; } println!("{}", ret); } but only because Rust, unlike C or C++, will enforce that `ret` is assigned exactly once before it is used.

Supermancho 5y ago
> IMHO the original version without the mutable local variable was much more obvious. In any language. This is not about obviousness. You can write the same expression lots of different ways, which are all obvious. This is still a question about testable paths and code hygiene. Nesting the same function call in both branches of if/else unintentionally obscures that it's not a conditional printf call. If you're going to write: printf(a == b ? "equal" : "not"); you don't need the function at all, completely missing the point. Your second example is definitely better in Rust.
- nybble41 5y ago
  > Nesting the same function call in both branches of if/else unintentionally obscures that it's not a conditional printf call. And the version with mutation masks the fact that the initial value assigned to `ret` may or may not be used, which seems like a very similar problem to me. Plus it involves mutation, which IMHO significantly increases the complexity of any code. Unfortunately C offers limited options here; as cryptic and controversial as it may be, the ternary operator is the only (non-hackish) way to make an expression which selects between two or more values based on a boolean condition without either the possibility of failing to assign a value or requiring a "default" value which at least some paths through the function won't use. (What if there is no reasonable default value and all the required branches are expensive to compute?) > If you're going to write: … printf(a == b ? "equal" : "not"); … you don't need the function at all, completely missing the point. The function still encapsulates the specific comparison and the way the result is reported—exactly like the other versions—so I wouldn't say it's completely useless despite containing only one line of code. This is, of course, merely a toy example. You could assign the string to a named const variable if you wanted to make it slightly more self-documenting, though I question whether that would make this particular code any more readable.