Bidirectional for?

[Aranince]

Relating to my previous post, I have a function that will order the list of tasks in descending or ascending order by priority. So I merged ascending and descending functions into one bidirectional one, but I'm having a bit of trouble. The for loop works perfectly descending. When ascending, it stops at 3. I'm assuming it has to do with the != operator being wonky when going in that direction?


int start;
int goal;

if(sort == SORT_ASCENDING)
{
start = 0;
goal = 5;
}
else if(sort == SORT_DESCENDING)
{
start = 5;
goal = 0;
}

for(int priority = start; priority != goal; )
{

// ...
if(sort == SORT_ASCENDING)
{
priority++;
}
else if(sort == SORT_DESCENDING)
{
priority--;
}
}

[lee1210]

that code itself does not seem to have a problem. With that said, a few suggestions:
Compile with -g, fire up gdb, and break in your function, set display on priority and goal, then step through and see when things go bad.
Don't get so complex with your loop control variables and conditionals. Loop from 0-4, 1-5, whatever, and use the current value to determine the priority. In one case subtract from 5 or 6, in the other use the number as-is.

-Lee

[Aranince]

Ok, thanks. Something like this? Ascending works, descending does not. If my math is correct, this should work.

(-5 + 0 = -5) * 1 = 5
(-5 + 1 = -4) * 1 = 4
(-5 + 2 = -3) * 1 = 3

right?


int mod = -5;

if(sort == SORT_ASCENDING)
mod = 0;

for(int priority = 0; priority <= 5; priority++ )
{
// for...
if((*iter)->getPriority() == (mod + priority)*1)
{
//...
}

//}

}

[Berlepsch]

A small thing that I noticed in your original code: what happens if sort is neither SORT_ASCENDING or SORT_DESCENDING ?

Unless sort is an enum type with only these two values, you might get stuck in an infinite loop by accident.

[sushi]

Curious. Why not keep them separate?

[itickings]

Ok, thanks. Something like this? Ascending works, descending does not. If my math is correct, this should work.

(-5 + 0 = -5) * 1 = 5
(-5 + 1 = -4) * 1 = 4
(-5 + 2 = -3) * 1 = 3

right?


As far as my math is concerned, -5 * 1 equals -5...

Regarding your original code, do you account for the fact that the loop in one case contains the values 0, 1, 2, 3 and 4, and in the other case 5, 4, 3, 2 and 1?

If you need the values to be 1, 2, 3, 4, 5 and 5, 4, 3, 2, 1, consider the following:
if(sort == SORT_ASCENDING)
{
start = 1;
goal = 6;
}

[lee1210]

Ok, thanks. Something like this? Ascending works, descending does not. If my math is correct, this should work.

(-5 + 0 = -5) * 1 = 5
(-5 + 1 = -4) * 1 = 4
(-5 + 2 = -3) * 1 = 3

right?


int mod = -5;

if(sort == SORT_ASCENDING)
mod = 0;

for(int priority = 0; priority <= 5; priority++ )
{
// for...
if((*iter)->getPriority() == (mod + priority)*1)
{
//...
}

//}

}

Hm, not quite. I was thinking:
for(int control = 1; control <=5; control++) {
if(SORT_DESCENDING == sort) {
priority = 6 - control;
} else if (SORT ASCENDING == sort) {
priority = control;
} else {
fprintf(stderr,"Invalid sort direction. Must call with SORT_DESCENDING or SORT_ASCENDING\n");
return NULL;
}
...
}

This could be collapsed to a ternary if, but i think it would reduce clarity. It would have to assume if SORT_DESCENDING is false, SORT_ASCENDING is true. Even if you did not do this, I think it might be better to have a bool that indicates ascending/descending, so you only have two discrete values. Enum, as Berlepsch pointed out, would also work. if you really wanted to, with sort being replaced with a sort_desc boolean, a ternary if would look like:
priority = sort_desc ? 6 - control : control;

Also, what you were looking for in place of *1 with the method you were trying to use would be abs(). I still think it would be clearer to calculate it differently, but:

if(sort == SORT_DESCENDING) {
mod=-6;
} else if (sort == SORT_ASCENDING) {
mod=0;
} else {
//error
return;
}
for(control = 1; control <=5; control++){
priority = abs(mod + control);
...
}

would work and use something similar to the logic you were implementing.

-Lee

[toddburch]

I would have coded something more along the lines of...


int start;
int goal;
inc increment ;


if(sort == SORT_ASCENDING) {
start = 0;
goal = 5;
increment = 1 ;
} else {
start = 5;
goal = 0;
increment = -1 ;
}

for (int priority = start; priority != goal; ) {
// ...
priority += increment ;
}

[lee1210]

I would have coded something more along the lines of...


int start;
int goal;
inc increment ;


if(sort == SORT_ASCENDING) {
start = 0;
goal = 5;
increment = 1 ;
} else {
start = 5;
goal = 0;
increment = -1 ;
}

for (int priority = start; priority != goal; ) {
// ...
priority += increment ;
}



At this point, why not make make priority += increment the third clause in the for, so it more closely resembles the standard for structure?

That would be my goal. Make the loop control variable, condition, and control variable modification as "standard" as possible so it's easy to follow the loop iterations, instead of looking all over to see what is going to result in the loop terminating, etc.

If you wanted to be able to do more than just the two sorts we're discussing, you could take this tact, instead:


int asc_prios[] = {1,2,3,4,5};
int desc_prios[] = {5,4,3,2,1};
int ext_asc_prios[] = {3,2,4,1,5};
int ext_desc_prios[] = {5,1,4,2,3};
int *prio_list = NULL;
switch(sort) {
case(SORT_ASCENDING):
prio_list = asc_prios;
break;
case(SORT_DESCENDING):
prio_list = desc_prios;
break;
case(EXTREMITY_ASCENDING):
prio_list = ext_asc_prios;
break;
case(EXTREMITY_DESCENDING):
prio_list = ext_desc_prios;
break;
default:
fprintf(stderr,"Invalid sort sent to task prioritization function.");
return null;
//or throw InvalidSortOrder exception, etc., since this is C++
}
for(int control = 0; control < 5; control++) {
...
for(...)
if((*iter)->getPriority() == prio_list[control])
}


Now you have the flexibility to add new sorts easily, without modification of the control logic in your nested for loops, etc. and your loop has the standard structure, and the flow control is easy to follow.

-Lee

[lee1210]

nt

[HiRez]

I would have coded something more along the lines of...


int start;
int goal;
inc increment ;


if(sort == SORT_ASCENDING) {
start = 0;
goal = 5;
increment = 1 ;
} else {
start = 5;
goal = 0;
increment = -1 ;
}

for (int priority = start; priority != goal; ) {
// ...
priority += increment ;
}Or to simplify even a little more:
for (int priority = start; priority != goal; priority += increment) {
// ...
}Of course that's only going to work if increment is 1 or -1, you can't skip the goal. I agree that the two are probably best left as separate loops though, unless they're really complex (duplicating a lot of code) or if this is just a mental programming exercise.

[toddburch]

Yes, I like the "priortity += increment" in the for loop too.

[Aranince]

Curious. Why not keep them separate?

Less code? Reduce code reuse...the code is exactly the same minus the countdown/countup. Thanks for your suggestions, I'll post back if I have any further questions or problems.