Mysterious problems with a linear search.

[shobits1]

Well even if he provides the full source, I wouldn't want to check that >20k chars code. He must be able to trim down the program while maintaining the problem he describes.

No one wants to check 20k chars code (which is about 700 line), but the process is the same as hunting bugs in even bigger codes.
even more maybe with full code spotting the problem maybe easier.

[Thaddy]

How do you instantiate or allocate mainlist? It looks like it isn't.

[Thaddy]

Well even if he provides the full source, I wouldn't want to check that >20k chars code. He must be able to trim down the program while maintaining the problem he describes.

Well, I would. It's a max 2 minute read (given 700 lines and not 700k lines) and I will probably find the basic errors. Most likely allocation of object or structure errors.
I don't say I will solve the bug in that time, but an experienced programmer WILL spot the obvious, so that means that you would too
I just ran into an example of that kind of sourcecode review in the bug tracker for FPC and that was done in three minutes including working but still bad code based on submitter's example. But at least now he has something that doesn't leak or crash otherwise

[benwrcoder]

http://pastebin.com/nXNfT5fX

Here. I trimmed some of the code that was absolutely unconnected to the issue, so it's down to ~700 lines.

Sorry to be a pain, it's just such a frustrating bug.

[Thaddy]

First observations (5 seconds). Example goes for ALL the lists, not just Poultry:

Code: Pascal  [Select][+][-]

1. procedure Sortlist_p(var List:poultrylist);  // should be ok, but better:
2. procedure Sortlist_p(const List:poultrylist);    
3.  

In the same code you don't test for assignment of the list. Better make sure:

Code: Pascal  [Select][+][-]

1. if Assigned(List) then  // you should bail out if there is nothing to do
2. begin
3.    first:=0;
4.    last:=List.Count-1; // .... this can become -1. Did you mean that?
5.    For CurrentPtr:= First+1 to Last do // ... And if the list is empty this causes an access violation.
6.    begin
7.         CurrentVal:= List[CurrentPtr];  // I mean BOOM!!
8.         Pointer:= CurrentPtr-1;
9.         while (pointer>-1) and (List[pointer].GetID() > CurrentVal.GetID) do
10.         begin
11.              List[Pointer+1]:=List[Pointer];
12.              pointer:=Pointer-1 ;
13.         end;
14.         List[pointer+1]:=CurrentVal;
15.    end;
16. end
17. else Raise ESomeKindOfApplicableException; // May be useful...
18.  
19.  

And this kind of code is wrong:

Code: Pascal  [Select][+][-]

1. For CurrentPtr:= First+1 to Last do  // What if the count is zero or 1?
2.  

[edit after some time]
Throughout the code you assume there are at least two items in any list you use. That is wrong. It can be one or empty. Fix that and it will probably work Let me know. (I tested some of the code).
And check all assignments. If for example the Mainlist is not created you will get the behavior you described, i.e. random pointer values.

If you sort using your sort routines and there are 0 or 1 items to sort, the behavior is also undefined. So at least sort when there is something to sort: Test if count > 1.

Use const when possible instead of var. It removes an indirection and it guards against accidentally double assignments and to a certain extend makes uninstantiated objects more visible.

[edit2]

And why are you writing your own sort routines when TFPGObjectList is derived from TFPSList which has already a sort routine?.

And do you really like memory corruption? Where are these lists and local objects  free'd? And should the objects be local? (I don't think so....)

Code: Pascal  [Select][+][-]

1. procedure TAddform.FormCreate(Sender: TObject);
2. var
3.   newPoultryObj:PoultryObj;
4.   newDiseaseObj:DiseaseObj;
5. begin
6.  
7.    mainlist:=poultrylist.Create();
8.    diseases:=diseaselist.Create();
9.    poultrydisease:= birddiseaselist.Create();
10.    poultrycoop:=PCList.Create();
11.  

Where's your form destroy?

[Thaddy]

In general there are way too many mistakes in your code- some big, many small - to solve them all.
I would start improving your code by the above pointers, but even better would be to skip that custom sort code altogether. That is where most of the mistakes are and it is also obviously unnecessary since your lists are all TFPSList descendants. So write a compare function for TFPSList sort instead, The internal quicksort is way better than your code is. And a compare function is just that, nothing complex. Also note that there is a default enumerator for the SList, so you could use

Code: Pascal  [Select][+][-]

1. if x in list

in a lot of places

But also don't forget to test for assignment, test for the count and never access an item directly if you are not sure it actually exists.

Code: Pascal  [Select][+][-]

1. PoultryObj

and the like are local to the procedure, btw. (dead give away for the original question) not members of the form. Move these two into form scope!

And, that, my friend, is all the time I will spend on that code

[last edit]
That was a reasonably complete code review and took about 15 minutes over the day. Mostly because I was not aware of some of the intricacies of the FGL (I still use a custom code base for these, most of the time).

[shobits1]

1st let me congratulate you for the messed up code  .
Anyway like Thaddy pointed
1.I can see that you sort function is a complete mess

Code: Pascal  [Select][+][-]

1.    
2. while (pointer>=0) and (List[pointer].GetID() > CurrentVal.GetID) do
3. begin
4.    List[Pointer+1]:=List[Pointer];   // you are overwriting the next without saving it.
5.    pointer:=Pointer-1 ;
6. end;
7.  

2.also you are repeating the same code 3 times (not good).
3.binarySearch_p uses recursion which I really dislike; and can be changed to while/for loop.
4.findPID_real is practically the same as binarySearch_p  (but with the list locally defined).
5.maybe just me but your variables names are confusing; eg. why do you use `pointer`, `CurrentPtr`... when you are just referring to list item index (most programmers will use i, j, ....)

my advice is to rewrite the entire code with the consideration of :
1.Create a custom class list, with the necessary function (sort, find, Add, Del, Clear...).
2.Try not use generics.
3.When creating a sort function use the builtin sort and pass your TCompareFunc; Take look at this
4.Try not to duplicate the same or almost the same code.

[benwrcoder]

Okay so, thanks for the help, people. I appreciate you taking the time to look over my terrible, terrible procedures.

Just some answers and stuff.

1) I have to use custom sort/search routines because that's in the task brief. I really really wish I didn't have to because I've spent so much time having trouble with them.

And do you really like memory corruption? Where are these lists and local objects  free'd? And should the objects be local? (I don't think so....)
-snip-

2) I don't know what you mean by "where are the lists free'd". We've not been taught anything remotely similar to that, and a quick google showed no hint of what that might entail or look like, even on the pascal wiki
3) The objects below should be local because they're only used in that procedure. Why would they be global? They're temporary holders to make the initialisation easier to read.

1.I can see that you sort function is a complete mess

Code: Pascal  [Select][+][-]

1.    
2. CurrentVal:= List[CurrentPtr];
3.         Pointer:= CurrentPtr-1;
4. while (pointer>=0) and (List[pointer].GetID() > CurrentVal.GetID) do
5. begin
6.    List[Pointer+1]:=List[Pointer];   // you are overwriting the next without saving it.
7.    pointer:=Pointer-1 ;
8. end;
9. List[pointer+1]:=CurrentVal;
10.  

4) Isn't it not overwriting the next without saving because list[pointer+1] = list[currentPtr], which was saved to currentVal?

2.also you are repeating the same code 3 times (not good).

5) I can't really see any other way to do it, seeing as all 3 are sorting different types of list, so the parameters have to be set differently.

3.binarySearch_p uses recursion which I really dislike; and can be changed to while/for loop.
4.findPID_real is practically the same as binarySearch_p  (but with the list locally defined).

6) findPID_real is actually the replacement for binarySearch_p. I fail to see how it's at all the same, binarysearch_p being a recursive binary search, and findPID_real being a plain linear search.

my advice is to rewrite the entire code with the consideration of :
1.Create a custom class list, with the necessary function (sort, find, Add, Del, Clear...).
2.Try not use generics.
3.When creating a sort function use the builtin sort and pass your TCompareFunc; Take look at this
4.Try not to duplicate the same or almost the same code.

7) I don't know what a custom class list is or why it would help, and I'm extremely too far into the project to rewrite everything from scratch.
8( What's a generic and why is it bad?


Again, thanks for taking the time to offer me help on all this.

[Thaddy]

And do you really like memory corruption? Where are these lists and local objects  free'd? And should the objects be local? (I don't think so....)
-snip-

2) I don't know what you mean by "where are the lists free'd". We've not been taught anything remotely similar to that, and a quick google showed no hint of what that might entail or look like, even on the pascal wiki

 When you create an object, instantiate a class it also needs to be destroyed at some point otherwise you have memory leaks. Somebody should have taught you that. It is simple:

Code: Pascal  [Select][+][-]

1. var
2.   L:TStringlist;
3. try
4.   L := TStringlist.Create;
5.   try
6.   ..... do something with the stringlist
7.   finally
8.     l.Free; // when you're domne using the stringlist, get rid of it!! This is essential. If you don't you make a BIG mistake. Really really big...
9.  end;
10.  

3) The objects below should be local because they're only used in that procedure. Why would they be global? They're temporary holders to make the initialisation easier to read.

Objects should also be disposed of, like above, but with Dispose(anObject) or if it has a destructor with anObject.Destroy.

4) Isn't it not overwriting the next without saving because list[pointer+1] = list[currentPtr], which was saved to currentVal?

Can you actually count? There are more places than one where you write code like List[pointer +1].
If the list is empty THERE IS NO ALLOCATED MEMORY for pointer + 1. It simply doesn't exist. BIG mistake.

5) I can't really see any other way to do it, seeing as all 3 are sorting different types of list, so the parameters have to be set differently.

You don't need your sort. You have to write a simple compare function and use the sort that is already provided by the list.

Again, thanks for taking the time to offer me help on all this.

I hope you will take advice. You really need to learn a lot about object pascal and we are here to help you.