typo bug in appointment search?

[alkhaef]

Lines ~ 127-135 from AppointmentL.cs read:

Code: Select all

						//match found
						ALresults.Add(dayEvaluating+timeFound);
					}//for p	
					if(aptIsMatch){
						break;
					}
				}
				dayEvaluating=dayEvaluating.AddDays(1);//move to the next day

I'm not sure what the break statement is trying to achieve, but I'm guessing the idea is to stop trying other providers if you find one already for that time slot. Is this correct? In that event, I believe there's a typo, and the code should be modified to look like below:

Code: Select all

						//match found
						ALresults.Add(dayEvaluating+timeFound);
						if(aptIsMatch){ //Unneccessary check?  This looks like a tautology...
							break;
						}
					}//for p	
				}
				dayEvaluating=dayEvaluating.AddDays(1);//move to the next day

I'm finding that the code as written has the side-effect that if only one provider is selected to search (or probably also if the LAST provider matches an appointment), it only finds one time slot per day (b/c the break statement is breaking the iteration through the slots in the day, not the providers).

Additionally, maybe the break doesn't need to be inside an if condition, because all branches that arrive to that if statement should have aptIsMatch set to true.

Best regards,

[jordansparks]

It's intentionally designed to keep more than one match from being added for each day. I don't see a problem with the current code. I made a comment there for future reference.

[alkhaef]

Hello Dr. Sparks,

Thanks for clarifying. I initially reported this because I noticed the following behavior:

- When multiple providers are selected, you usually get multiple slots per day.
- When a single provider is selected, you only get one.

I had to guess which one was intended, and the one that made more sense to us (and the one we want) was the first one, so I assumed the bug was in the single-provider case. According to what you're saying, my assumption was inside-out.

So to make it behave as intended, one option is the following patch (which I hope you don't use, as I mention below ):

Code: Select all

--- AppointmentL.cs.head-2010-09-10     2010-09-10 14:55:08.643210100 -0700
+++ AppointmentL.cs     2010-09-10 15:45:13.835155000 -0700
@@ -82,8 +82,11 @@
                                //step through day, one increment at a time, looking for a slot
                                pattern=ContrApptSingle.GetPatternShowing(apt.Pattern);
                                timeFound=new TimeSpan(0);
-                               for(int i=0;i<provBar[0].Length;i++){//144 if using 10 minute increments
-                                       for(int p=0;p<providers.Length;p++){
+
+                               bool findMoreMatchesToday=true;
+
+                               for(int i=0;findMoreMatchesToday && i<provBar[0].Length;i++){//144 if using 10 minute increments
+                                       for(int p=0;findMoreMatchesToday && p<providers.Length;p++){
                                                //assume apt will be placed here
                                                aptIsMatch=true;
                                                //test all apt increments for prov closed. If any found, continue
@@ -126,11 +129,9 @@
                                                }
                                                //match found
                                                ALresults.Add(dayEvaluating+timeFound);
+                                               findMoreMatchesToday=false;
                                                Plugins.HookAddCode(null,"AppointmentL.GetSearchResults_postfilter",ALresults,providers[p],apt);
                                        }//for p        
-                                       if(aptIsMatch){//if a match found in this day
-                                               break;//don't add any more matches for this day
-                                       }
                                }
                                dayEvaluating=dayEvaluating.AddDays(1);//move to the next day
                        }

However, as I mentioned, we would prefer the search to return all options for a given day. Since it seems like we might be in the minority and that's not gonna happen, I'd like to request the ability to change the behavior within the hook I requested. The following alternate patch would squash the original bug and give me that flexibility all at the same time.

Code: Select all

--- AppointmentL.cs.head-2010-09-10     2010-09-10 14:55:08.643210100 -0700
+++ AppointmentL-allowoverride.cs       2010-09-10 15:51:14.571302200 -0700
@@ -82,8 +82,11 @@
                                //step through day, one increment at a time, looking for a slot
                                pattern=ContrApptSingle.GetPatternShowing(apt.Pattern);
                                timeFound=new TimeSpan(0);
-                               for(int i=0;i<provBar[0].Length;i++){//144 if using 10 minute increments
-                                       for(int p=0;p<providers.Length;p++){
+
+                               object[] findMoreMatchesToday=new object[]{true};
+
+                               for(int i=0;((bool)(findMoreMatchesToday[0])) && i<provBar[0].Length;i++){//144 if using 10 minute increments
+                                       for(int p=0;((bool)(findMoreMatchesToday[0])) && p<providers.Length;p++){
                                                //assume apt will be placed here
                                                aptIsMatch=true;
                                                //test all apt increments for prov closed. If any found, continue
@@ -126,11 +129,9 @@
                                                }
                                                //match found
                                                ALresults.Add(dayEvaluating+timeFound);
-                                               Plugins.HookAddCode(null,"AppointmentL.GetSearchResults_postfilter",ALresults,providers[p],apt);
+                                               findMoreMatchesToday[0]=false;
+                                               Plugins.HookAddCode(null,"AppointmentL.GetSearchResults_postfilter",ALresults,providers[p],apt,findMoreMatchesToday);
                                        }//for p        
-                                       if(aptIsMatch){//if a match found in this day
-                                               break;//don't add any more matches for this day
-                                       }
                                }
                                dayEvaluating=dayEvaluating.AddDays(1);//move to the next day
                        }

Yes, it's ugly - enclosing the bool within an object array, but AFAIK, it's the cleanest way to achieve this within the callback framework as we have it.

Thanks again,

[jordansparks]

And why wouldn't we use a List<bool> instead of an object array?

[alkhaef]

Ah! Great idea. That makes the syntax quite cleaner. I tested it and revised the patch below:

Code: Select all

    --- AppointmentL.cs.head-2010-09-10     2010-09-10 14:55:08.643210100 -0700
    +++ AppointmentL-allowoverride.cs       2010-09-10 15:51:14.571302200 -0700
    @@ -82,8 +82,11 @@
                                    //step through day, one increment at a time, looking for a slot
                                    pattern=ContrApptSingle.GetPatternShowing(apt.Pattern);
                                    timeFound=new TimeSpan(0);
    -                               for(int i=0;i<provBar[0].Length;i++){//144 if using 10 minute increments
    -                                       for(int p=0;p<providers.Length;p++){
    +
    +                               List<bool> findMoreMatchesToday = new List<bool>(); findMoreMatchesToday.Add(true);
    +
    +                               for(int i=0;findMoreMatchesToday[0] && i<provBar[0].Length;i++){//144 if using 10 minute increments
    +                                       for(int p=0;findMoreMatchesToday[0] && p<providers.Length;p++){
                                                    //assume apt will be placed here
                                                    aptIsMatch=true;
                                                    //test all apt increments for prov closed. If any found, continue
    @@ -126,11 +129,9 @@
                                                    }
                                                    //match found
                                                    ALresults.Add(dayEvaluating+timeFound);
    -                                               Plugins.HookAddCode(null,"AppointmentL.GetSearchResults_postfilter",ALresults,providers[p],apt);
    +                                               findMoreMatchesToday[0]=false;
    +                                               Plugins.HookAddCode(null,"AppointmentL.GetSearchResults_postfilter",ALresults,providers[p],apt,findMoreMatchesToday);
                                            }//for p       
    -                                       if(aptIsMatch){//if a match found in this day
    -                                               break;//don't add any more matches for this day
    -                                       }
                                    }
                                    dayEvaluating=dayEvaluating.AddDays(1);//move to the next day
                            }

Thanks,

[jordansparks]

Ok, added to head. Too dangerous to backport. And 7.3 should be coming out soon anyway.

[alkhaef]

Hmm... Too bad, but I guess I'll just roll out my plugin against 7.2 and make it forward-compatible to head. I'll just tell the staff to select two providers when searching in the meantime and take advantage of the "bug" . Anyway, thanks a bunch for the hooks.

Speaking of which... Cool! 7.3 is on its way, huh? Looking forward to seeing the progress.

Thanks,