Bidirectional for?

Discussion in 'Mac Programming' started by Aranince, Oct 4, 2008.

  1. Aranince macrumors 65816

    Joined:
    Apr 18, 2007
    Location:
    California
    #1
    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?

    Code:
        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--;
            }
        }
     
  2. lee1210 macrumors 68040

    lee1210

    Joined:
    Jan 10, 2005
    Location:
    Dallas, TX
    #2
    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
     
  3. Aranince thread starter macrumors 65816

    Joined:
    Apr 18, 2007
    Location:
    California
    #3
    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?

    Code:
        int mod = -5;
        
        if(sort == SORT_ASCENDING)
            mod = 0;
        
        for(int priority = 0; priority <= 5; priority++ )
        {
            // for...
                if((*iter)->getPriority() == (mod + priority)*1)
                {
                    //...
                }
                
            //}
            
        }
     
  4. Berlepsch macrumors 6502

    Berlepsch

    Joined:
    Oct 22, 2007
    #4
    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.
     
  5. sushi Moderator emeritus

    sushi

    Joined:
    Jul 19, 2002
    Location:
    キャンプスワ&#
  6. itickings macrumors 6502a

    itickings

    Joined:
    Apr 14, 2007
    #6
    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:
    Code:
    if(sort == SORT_ASCENDING)
    {
      start = 1;
      goal = 6;
    }
     
  7. lee1210 macrumors 68040

    lee1210

    Joined:
    Jan 10, 2005
    Location:
    Dallas, TX
    #7
    Hm, not quite. I was thinking:
    Code:
    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:
    Code:
    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:
    Code:
    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
     
  8. toddburch macrumors 6502a

    Joined:
    Dec 4, 2006
    Location:
    Katy, Texas
    #8
    I would have coded something more along the lines of...

    Code:
    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 ;
    }
     
    
     
  9. lee1210 macrumors 68040

    lee1210

    Joined:
    Jan 10, 2005
    Location:
    Dallas, TX
    #9
    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:

    Code:
    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
     
  10. lee1210 macrumors 68040

    lee1210

    Joined:
    Jan 10, 2005
    Location:
    Dallas, TX
  11. HiRez macrumors 603

    HiRez

    Joined:
    Jan 6, 2004
    Location:
    Western US
    #11
    Or to simplify even a little more:
    Code:
    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.
     
  12. toddburch macrumors 6502a

    Joined:
    Dec 4, 2006
    Location:
    Katy, Texas
    #12
    Yes, I like the "priortity += increment" in the for loop too.
     
  13. Aranince thread starter macrumors 65816

    Joined:
    Apr 18, 2007
    Location:
    California
    #13
    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.
     

Share This Page