3

I am trying to make two loops into one. This Loop should go through the array from the beginning to the end and otherwise. However my increment is not correct. Can anyone help? Thank you in advance.

for (int i = ((asc == true) ? 0 : calendar.Length - 1);
    ((asc == true) ? i < calendar.Length : i > 0); 
    (asc==true) ? i++ : i--)
juharr
  • 31,741
  • 4
  • 58
  • 93
  • 9
    good lord that's hard to read – Jonesopolis Nov 10 '14 at 19:21
  • 11
    *Why* are you trying to make two loops into one? And why are you comparing with `true` instead of just using `asc` as the condition? Fundamentally, you're trying to abuse the conditional operator... – Jon Skeet Nov 10 '14 at 19:21
  • 6
    I would be very upset if I saw this code in production. Just use `Reverse` for goodness sake. – BradleyDotNET Nov 10 '14 at 19:22
  • 8
    short is not always the best solution ;) – silent Nov 10 '14 at 19:22
  • 1
    Always post the full error message(s). Your code does not compile, that's not very clear from "my increment is not correct". – H H Nov 10 '14 at 19:46
  • You are trying to make compact code but you have "== true" not once but 3 times? It's an entirely unnecessary clause if 'asc' is a Boolean. – Acey Nov 10 '14 at 19:57

4 Answers4

6

Personally, I find that very hard to read, as well as invalid (it won't compile) - because you're trying to use the conditional operator as a statement expression, when it's not. Personally, I'd write something like:

for (int i = 0; i < calendar.Length; i++)
{
    int index = asc ? i : calendar.Length - 1 - i;
    // Now use index...
}

Making three different aspects all conditional feels like a nasty way to go.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • 2
    I think you meant to subtract `i` at the end instead of `index`. – juharr Nov 10 '14 at 19:25
  • I don't see where the conditional op is used as a statement, `for(a;b;c)` requires 3 expressions. – H H Nov 10 '14 at 19:28
  • @Henk: the second is an expression, but I think the first and third have to be statements. Will check tonight. – Jon Skeet Nov 10 '14 at 19:32
  • You're kind of right, it should be "zero or more statement expressions". The OP's code does not compile, that wasn't clear. – H H Nov 10 '14 at 19:45
4

Jon offered a good option, but if principle:

for (int i = (asc ? 0 : calendar.Length - 1);
     asc ? i < calendar.Length : i >= 0; 
     i += asc?1:-1)
{
            //body
}
Evgeniy Mironov
  • 777
  • 6
  • 22
  • This does fix the fundamental issue with the original code. And the irritating one of comparing a `bool` to `true`. – juharr Nov 10 '14 at 19:32
  • 1
    To be fair, you're making an assumption that it's not a comparison against a System.Nullable. – lsuarez Nov 10 '14 at 19:33
4

Look at the C# reference of "for": http://msdn.microsoft.com/en-us/library/ch45axte.aspx

Specifically:

The iterator section defines what happens after each iteration of the body of the loop. The iterator section contains zero or more of the following statement expressions, separated by commas:

  • assignment statement
  • invocation of a method
  • prefix or postfix increment expression, such as ++i or i++
  • prefix or postfix decrement expression, such as --i or i--
  • creation of an object by using new
  • await expression

The expression: "(asc==true) ? i++ : i--" is none of these things.

Therefore, you'd want the assignment: i += (asc ? 1 : -1)

for (int i = ((asc) ? 0 : calendar.Length - 1);
      ((asc) ? i < calendar.Length : i >= 0); 
      i += (asc) ? 1 : -1)

Incidentally, as pointed out in comment, you'll probably want to look at index 0 in the condition, so your condition statement in the "descending" case should be i >= 0 (reflected in the code).

Griffin
  • 1,586
  • 13
  • 24
1

That for loop is... odd. It is also very hard to read. In the spirit of "better ways to do this", I would suggest just using Reverse:

IEnumerable<Day> collection = asc ? calendar : calendar.Reverse();

for (int i = 0; i < calendar.Length; i++)
{
    collection.ElemantAt(i);// This is the current element
}

//Or better, you are getting everything anyways:
foreach (Day day in collection)
{
}
BradleyDotNET
  • 60,462
  • 10
  • 96
  • 117