Linked List Class

[Pascaluvr]

My first ever OOP attempt.

Very basic at this stage....  includes:

Additem
InsertItem
DeleteItem
GetItem

http://www.mediafire.com/?1ui42qgyswl94hh

Look forward to any comments, suggestions etc.

[CaptBill]

Looking good.

I take you intended for the error screen dump when you press the free button, correct?

[Pascaluvr]

Looking good.

I take you intended for the error screen dump when you press the free button, correct?

Hi Captain,

Yes, when dealing with LinkLists (lots of FreeMem and GetMem) its easy to mess something up and get a memory leak.  You can turn that window off in Project Options ----> Compiler Options ---- Linking
Uncheck the Use HeapTrc option.

BTW.  My documentation skills are far worse than my programming skills.  Did the .Doc file make sense?

Thanks again for all your help.

[CaptBill]

Yeah, I am just now taking a peek at the files. Put a lot of work into docs, I see. Nice. You should plan on doing a 'My first taste of OOP with Lazarus' chronicle on the Wiki.

One suggestion: do 'strip symbols' and 'smartlinking' on the demo exe since you have the project files in you zip file. It will bring the size from 12mb to about 2.5mb. The zip file will be <1mb that way too.

[User137]

2 things come to mind:

Code: [Select]

Data:         Array of Char;I would have used Pointer type instead of array of char. That way you can have anything in the list and not ever touch the actual data that pointer refers to.

There should always be indentation after word begin. Not after function, procedure, constructor etc.

This looks awkward... See for example Forms unit in the uses list, to see what general way is.

Code: [Select]

    For i := 0 to LL.ICount-1 do
      begin
      SetLength(item, 100);
      LL.GetItem(i, @item[1]);
      Memo1.Lines.Add(Item);
      end;
    end; // this end; actually ends the procedure...

[Pascaluvr]

2 things come to mind:

Code: [Select]

Data:         Array of Char;I would have used Pointer type instead of array of char. That way you can have anything in the list and not ever touch the actual data that pointer refers to.

The problem with that is that the actual data disappears when 'item' has been assigned a new value for the next AddItem = the initial Item will now point to the new value, not the value it had at the time it was added.  Even worse, if the item is a long string, that pointer may now be to an unallocated part of memory and thus cause our favourite SIGSEGV error.  Sorry, but the data needs to be copied to the allocated memory and 'array of char' is perfect for copying unknown data types.

There should always be indentation after word begin. Not after function, procedure, constructor etc.

"There should always be........ " uuummmmmm if that is the case, why didn't Freepascal throw ou an error message, or a warning, or at least a hint?

This looks awkward... See for example Forms unit in the uses list, to see what general way is.

I am very sorry my style looks awkward to you, but since I have been programming for over 40 years, have seen many different coding styles and layouts ...  plus I program for a hobby and thus will program in a way that I find the most readable for ME to go through and debug.

I once had someone chastise me for using a semicolon on the last statement in a procedure before the end; statement.  He said it wasn't necessary and so I shouldn't use it.  I replied that 'white space' is unnecessary so he shouldn't use that.  Hi reply: 'But thats different!'.

There are lots of great standards out there that are worth following.  If you program for a career, you follow the standards of the company you work for (and if you get out and about, you will find there are many different 'standards').

I like my standards and since I work for myself, I will follow the standards I have set for myself.  My apologies to anyone who doesn't like them.

[Pascaluvr]

Yeah, I am just now taking a peek at the files. Put a lot of work into docs, I see. Nice. You should plan on doing a 'My first taste of OOP with Lazarus' chronicle on the Wiki.

ROFLMAO .....  where should I start?  Perhaps with the well-meaning people who were bugging the sh!t out of me when I just wanted some sensible answers?  or ...  LOL...  the part where I was ready to throw the computer out the window .... or maybe the part where we discovered that a unit that wasn't even being used caused a SIGSEGV on form close.  (You were expecting a 'nice' article I take it?

One suggestion: do 'strip symbols' and 'smartlinking' on the demo exe since you have the project files in you zip file. It will bring the size from 12mb to about 2.5mb. The zip file will be <1mb that way too.

You are probably correct here.  Had this been an application that was of some use, I probably would have done so.  When I download something like this I tend to use the 'demo' code to try variations that I need, and let me assure you, when I do, I want every bit of debugging info available to me.  Admittedly, since its all there, the recompile would have included it.  A few years ago I would have done so, mainly because of the download speed.  I have been spoiled over the last few years with a super fast internet connection that a 12mb file downloads quicker than I can close the window that started it.  I guess not all have that tho, so I should think about such things.

Thanks for the feedback

[CaptBill]

Hahaha

The funny thing is, is that your 'first date with OOP' is just like everyone elses was. It is very frustrating. I know it was for me. Go look at some of my first posts on this forum. So yeah, that should be part of the 'chronicle' for sure, come with the territory.

It's actually quite comical looking back.(my trip that is)

There is definitely an unexpected 'hill' you got to overcome, but bear with it. You will be glad.

Do you want me to bug the shit out of you or not?

[Pascaluvr]

"Now, question I have is this, are you in the mood right now for me to point out a few things or not? I just spent quite a while putting some constructive suggestions for you. There were a few things you missed on the OOP side."

LOL yes Bill, I am always happy to hear constructive criticism.  I know I can be a grumpy old PITA at times, but I don't expect you to be telling me that I put one too many spaces in front of my 'begin' statement.

If its referring to learning more about OOP, you have my undivided attention

[CaptBill]

Ok, good. (I spent about an hour on this, then I saw you might not be in the mood anymore, hahaha)

I am just going to give you some 'structural' analysis of how you designed your class vs. the 'convention', just to give some helpful tips with the OOP part, the 'framing'... Your coding looks great. Were simply shuffling the class definition a bit. It will become second nature to you soon. Has noting to do with your programming skills (which are way above mine). So please take this as 'constructive', pardon the pun.

OOP is frustrating at first. Your structural hierarchy just needs a little shuffling is all. And OOP is all about structure and 'conventions' get you 80% there. So just bear with it. It will all make sense before you know it.

Your setitem and getitem methods should be in the 'public' section and not the 'private' section. This gives you a 'black box' like seclusion from the outside 'world', OOP encapsulating it inside. And also you can combine these two functions as well in a single property.

Also looks like you should place your methods last in the definition instead of first. This is the convention anyway. Takes a while to ingrain the concept of the typical structure.

I didn't test this but here is the 'conventional' outline reshuffled a little (I think the header may be wrong with the items property, but shows the idea):

I gotta hit the sack. See you tomorrow.

Code: [Select]


   tLinkedList = Class

    Private
      First,
      Last,
      Curr:     ptrItemRec;
      ItemCount:            LongInt;
      ItemSize:             LongInt;

    Protected

      Function DeleteItem(ItemNo: LongInt): Boolean;
      Function GetItem(ItemNo: LongInt; Item: Pointer): Boolean;
      Function InsertItem(ItemNo: LongInt; Item: Pointer; Len: LongInt): Boolean;

    Public

      Constructor Create;
      Destructor Destroy; Override;
      Procedure Clear;
      Procedure AddItem(Item: Pointer; len: LongInt);//is this a dup of InsertItem?

      Property ICount:      LongInt Read ItemCount;
      Property ISize:       LongInt Read ItemSize;
      Property items:      LongInt read GetItem write InsertItem;

     
    end;

[CaptBill]

Looks like I missed part of the 'property SetItem' explanation. I know the header is wrong. Idea is to isolate the public from the protected and private sections. Simple once you do it once or twice. A pain to wrap your mind around at first.

Good night

[Pascaluvr]

Yeah,  I gotta hit the hay too.  But just to give you something to wake up to:

If all methods (except AddItem) are protected, doesn't that mean nobody can access them?

I'll read it again tomorrow,  prolly too much amber fluid for one nite

[Pascaluvr]

Bill,
Part 2 for you tp wake up to.

If we consider just PRIVATE and PUBLIC, you realise that we had that before OOP.  If it was Public, we had a forward declaration before the IMPLEMENTATION statement.  If it was PRIVATE we didn't.  Now we declare them before the IMPLEMENTATION and say they are PRIVATE so we can pretend we didn't!

[User137]

Even if you're not using OOP, you can't define private types in implementation section. You can have an object instance of a class in implementation var section.

The problem with that is that the actual data disappears when 'item' has been assigned a new value for the next AddItem = the initial Item will now point to the new value, not the value it had at the time it was added.  Even worse, if the item is a long string, that pointer may now be to an unallocated part of memory and thus cause our favourite SIGSEGV error.  Sorry, but the data needs to be copied to the allocated memory and 'array of char' is perfect for copying unknown data types.

(Firstly, not saying you should do this, but just in general.)
That is not entirely true, but depends on the data types. I know string in this case would be problematic, but still possible. Maybe allocate PChar or something? If you create an object, or allocate memory in a procedure and don't free it, it will remain in the memory forever. Imagine a record that has 1 million bytes of data, pointer references and links to class objects. The list could have 100 of these records. If you regard them as pointers, the speed gain is huge. The time it takes to simply swap pointers instead of doing all memory copying, is much faster, especially on unknown data types.

"There should always be........ " uuummmmmm if that is the case, why didn't Freepascal throw ou an error message, or a warning, or at least a hint?

Nah, nobody will force you to code certain way. However, you are seeking help from other programming community, and its only polite to present your code in a readable way. By readable, i mean if it's difficult to distinct what part of code belongs to which block, then not only is it much harder to find errors, its simply more difficult to even understand what the code does.

Yes, i put semicolon at end of code blocks too. It's weird that it's not required... Either way, it won't generate any more CPU code so its just matter of taste. I might want to add more code after that last line, and then could forget to add that semicolon later.

[eny]

My first ever OOP attempt.

You managed to create a working program with no memory leaks.
That is an accomplishment in itself 

Very basic at this stage....  includes:

A useful next step would be to make use of what is available in the 'Lazarus toolbox'.
Instead of building a linked list (what you probably have done many times before) you could use a TObjectList (unit contnrs) to store the data.
Make a class of ItemRec with it's own constructor and destructor and the methods that do all data handling.
One of the basics of OO is data encapsulation i.e. put the data you need and the methods that manipulate it in one and the same class.

Nah, nobody will force you to code certain way. However, you are seeking help from other programming community, and its only polite to present your code in a readable way. By readable, i mean if it's difficult to distinct what part of code belongs to which block, then not only is it much harder to find errors, its simply more difficult to even understand what the code does.

Indeed.  Most time is spent reading code, not writing it.
And the easier it is to read, the easier it is to understand (and spot possible errors or better, assess that the code is indeed correct).

Yes, i put semicolon at end of code blocks too. It's weird that it's not required...

In pascal the semicolon is not so much a terminator as for example in C, but it's used to separate statements.
The good thing is that the extra semicolons don't do any harm.

Sorry, but the data needs to be copied to the allocated memory and 'array of char' is perfect for copying unknown data types.

'Data' is merely a placeholder to find the first memory location after the variables prev, next and ilen.
The type is irrelevant.
Actually you declared a dynamic array which is not the most obvious choice; it suggests that you somewhere have a need for the dynamic nature of that attribute.
A more common approach is to use a static array like 'array[0..1] of byte'.

TLinkedList.Curr should be declared as a local variable in the methods that use it, not an object attribute. It does not contain any information about the state of an object.
TLinkedList.ItemSize should not be there because of the flexible way the list is built: the sizes of the elements in the linked list can all be different.