Sign in to follow this  
Spa8nky

[C#] Is this incrementation ternary method sensible?

Recommended Posts

Spa8nky    230
I've got the following for obtaining the next frame index for my animation:
// Use current frame
...

// Get the next frame
frame_Index = ++frame_Index > frame_Finish ? frame_Start : frame_Index;
Is it sensible to use ++frame_Index by incrementing the value for frame_Index so frame_Index is the correct value after the evaluation without needing to increment it again? Thanks.

Share this post


Link to post
Share on other sites
Telastyn    3777
While it might be syntactically correct, and it might even work reliably (honestly, I have no idea...), I would be less than pleased if I happened across that in code I had to work with.

Share this post


Link to post
Share on other sites
SiCrane    11839
I would personally use:

if (frame_Index >= frame_Finish) {
frame_Index = frame_Start;
} else {
++frame_Index;
}

Though I'd still find Telastyn's version acceptable.

Share this post


Link to post
Share on other sites
Wan    1366
I wouldn't mind a one-liner in this particular example, but I suspect I'm in the minority here.

Share this post


Link to post
Share on other sites
kloffy    1318
Quote:
Original post by Spa8nky
Thanks guys.

I must confess, my love for ternary operators is probably going to get me into trouble!

You're not alone, I cannot keep my fingers off them. Writing code as concisely as possible is fun (e.g. code golf...). However, I agree that one should abstain from using such contraptions in production code.

Share this post


Link to post
Share on other sites
theOcelot    498
I don't mind the ternary operator in this case, just using it in the same line as you increment a variable you're already assigning to. Even if that's well-defined in C#, it's still a little confusing.

Share this post


Link to post
Share on other sites
nullsquared    126
Or an even shorter version of Telastyn's code:

if (++frame_Index > frame_Finish)
frame_Index = frame_Start;

Personally that makes a lot more sense than any of the others; it says "if we increment the frame index and it turns out to be past the finish frame, then set it to the starting frame."

Share this post


Link to post
Share on other sites
Telastyn    3777
Quote:
Original post by nullsquared
Or an even shorter version of Telastyn's code:

if (++frame_Index > frame_Finish)
frame_Index = frame_Start;

Personally that makes a lot more sense than any of the others; it says "if we increment the frame index and it turns out to be past the finish frame, then set it to the starting frame."


Terseness is not a positive metric for code. I would refactor this just as quickly as the original example.

Share this post


Link to post
Share on other sites
Zipster    2365
For single-frame advancement:
frameIndex = (frameIndex < frameFinish) ? (frameIndex + 1) : frameStart;
However it's more useful if you handle the N-frame advancement case, of which single-frame advancement is just N=1:
frameIndex = Wrap<int>(frameIndex + N, frameStart, frameFinish);
Where Wrap<T>(T value, T min, T max) is a generic method that wraps value with respect to the range [min, max]. Our math library at work has the C++ equivalent and it's an extremely useful method to have around.

Share this post


Link to post
Share on other sites
Brother Bob    10344
Quote:
Original post by BFG
++frame_Index;
frame_Index %= frame_Count;

You assume that frame_Start is zero. If it is, then you can also use

frame_Index = (frame_Index + 1) % frame_Count;

as a fairly descriptive one-liner.

Share this post


Link to post
Share on other sites
nullsquared    126
Quote:
Original post by Telastyn
Terseness is not a positive metric for code. I would refactor this just as quickly as the original example.


Why? How is this:

++num;
if (num > 5)
num = 0;

any more clear than this:

if (++num > 5)
num = 0;

?

The second is even better because you'll coupling cause and effect, making it easier to read & understand.

Share this post


Link to post
Share on other sites
Telastyn    3777
Quote:
Original post by nullsquared
Quote:
Original post by Telastyn
Terseness is not a positive metric for code. I would refactor this just as quickly as the original example.


Why? How is this:

++num;
if (num > 5)
num = 0;

any more clear than this:

if (++num > 5)
num = 0;

?

The second is even better because you'll coupling cause and effect, making it easier to read & understand.


Because it's being 'clever'. Did you know before writing that code that ++ had higher operator precedence than greater-than? Do you know what operators you can swap with the greater-than and still get the expected order? Most importantly, will the person maintaining your code know that trivia?

It is always more clear to have a single line of code do a single operation. Sometimes that clarity is negligible. This is (imo) not even close to one of those cases. Mostly because that operator ordering trivia is needed to maintain the code with confidence. Partially because it sets off alarm bells for anyone who has done C or C++ for any period of time as Mike.Popoloski mentions.

Share this post


Link to post
Share on other sites
nullsquared    126
Quote:
Original post by Telastyn
Because it's being 'clever'.

Wow. Just writing C++ must make everyone really clever then.
Quote:
Did you know before writing that code that ++ had higher operator precedence than greater-than? Do you know what operators you can swap with the greater-than and still get the expected order? Most importantly, will the person maintaining your code know that trivia?

Yup, yup, and most importantly, yup. Otherwise they should be fired. What kind of idiot maintaining C++ code would not know that operator++ has a higher precedence than operator<?

Quote:

Mostly because that operator ordering trivia is needed to maintain the code with confidence.

Trivia?

While I do agree that breaking down some extremely complex statements can help readability immensely, your argument is null and void for my specific example. Anyone half decent with C++ will perfectly understand it.

Otherwise, we might as well stop being clever with other things, too. Short circuiting? Nope, too clever, someone might not know about it. Precedence of && vs. ||? Nope, too clever. Polymorphism? Nope, too clever - someone might see the base pointer and not know it points to a derived type.

C++? Nope, too clever. Lets drop C++ all together so we don't have silly arguments about how if (++num > 5) is too complex to understand.

Share this post


Link to post
Share on other sites
Mike.Popoloski    3258
We don't need you to give us an attitude in order for you to make your point. All Telastyn is saying is that the increment and decrement operators can be particularly troublesome bits of code in that it's easy to miss the intended results at first glance. Unlike other operators, their placement on either side of the variable can completely change the expression, so it's easier to skim code that has them split into discrete operations.

Anyways, I'm partial to code like this myself:

public Board(string input) : this(input.ToCharArray().Select(c => "Xx. -".Contains(c) ? '0' : c).Where(c => char.IsNumber(c)).Select(c => new Cell() { Value = c - '0' }))

or

foreach(var range in ranges) results.Add(ranges.Aggregate(range, (current, item) => (current.Contains(item.Lower) ^ current.Contains(item.Upper)) ? new Range(Math.Min(current.Lower, item.Lower), Math.Max(current.Upper, item.Upper)) : item.Lower < current.Lower && item.Upper > current.Upper ? item : current));

[grin]

Share this post


Link to post
Share on other sites
Zipster    2365
Quote:
Original post by BFG
++frame_Index;
frame_Index %= frame_Count;

Well yes, that works, but only if the starting frame is 0 as Brother Bob mentioned. But if we need it to work in the general case (which is a reasonable assumption given that the OP is using a frame_Start variable), then the wrapping logic becomes a bit more unsightly. OTTOMH:
public T Wrap<T>(T value, T min, T max)
{
return ((value - min) % (max - min + 1)) + min;
}

Not terribly horrendous, but certainly not something you'd quickly understand if you just stumbled upon it.

Share this post


Link to post
Share on other sites
Spa8nky    230
That's got me thinking. Has anyone got any good suggestions for a Wrap class:

public T Wrap<T>(T value, T min, T max)

I'm all for avoiding % (modulo) if possible!

Share this post


Link to post
Share on other sites
rip-off    10979
The thing about operator precedence is, it is easy to see what precedence the compiler will apply, but what the code does not show is the precedence the programmer intended.

I make a point of using parantheses for all non-trivial expressions so that my intent is clear.

As a side effect of this, I've gradually forgotten all the predence rules that aren't obvious or necessary. Every couple of months I come across an expression complex enough such that I need to reach for google to tell me the precedence of modulus relative to the other mathematical operators... but its rare enough that I don't seem to be able to commit them to memory.

Personally, I don't think it affects my code. I am a big fan of breaking down complex expressions into easy to *read* chunks. I would consider moving the increment into the conditional expression "trying to be clever", because for the sake of a rather minor extra line clarity is lost.

I can guarantee that even those who are familiar with those tricks take an order of magnitude more brain processing on the "clever" version than the simple, even though the total time spent thinking is tiny.

For me, readable programs make their intent clear at a glance, not make themselves short.

Share this post


Link to post
Share on other sites
nullsquared    126
Quote:
Original post by rip-off
[...] but what the code does not show is the precedence the programmer intended.


How so?

if (++n > 5) // clear intent: if we increase n and it becomes greater than 5
foo();
// post: n will have increased by 1


Now, the other argument:

if (n++ > 5) // not-so-clear intent: if we increase n and its old value is greater than 5
bar();
// post: n will have increased by 1

I must say that this second case is a little weird since the condition depends on n's old value, not the new one; as such, perhaps something like the following would be preferred:

if (n > 5)
{
n++;
bar();
}
// post: n may or may not have increased by 1

I can't really think of any good conditional where you would want to use post-fix ++. It is pretty useful in cases like this, though:

arr[i++] = r;
arr[i++] = g;
arr[i++] = b;

etc.

Really, like I said, this argument is null and void for any semi-decent programmer. While complex statements should indeed be broken down, if one has issues understanding what if (++n > 5) does or intends to do, then one needs to brush up.

Share this post


Link to post
Share on other sites
Guest
This topic is now closed to further replies.
Sign in to follow this