Need your expertise

[allanregistos]

Hi all,

I've been using Lazarus IDE now for the past months. My previous PL was vb.net, and even I don't like it, I've used it because its easy and fast to create apps. Now, I have created few utilities(LOBs) database applications using Lazarus for the company where I presently working.  I just want to make sure that my code is friendly to others, that is, not like a huge spaghetti code.
(Disclaimer: My knowledge of Objects is pretty vague when applied to Programming(OOP). So my programming method is mostly procedural.)

I did not use an database abstracting method to let my app work with any databases(because I have no knowledge how to do this yet),  also because I am a fan of PostgreSQL and since my code is in-house and not to be released in the public domain. I am interested from anyone's expertise if what I am doing with my code is at least acceptable.  Also, I just want to make sure that I am separating the GUI from the database layer.

Part of my work is the code snippet below. The code will let me delete a record from the database. I have separated the db queries in a separate unit(dbqueries). Most of my code resembles below.

My question is, can this code be accepted as doing the separation of GUI from the database layer?

Thank you...

Code: [Select]

     BoxStyle := MB_ICONQUESTION + MB_YESNO + MB_DEFBUTTON2;
      FMT_Message := 'Do you really want to delete: ''%s'' from the record?';
      FMT_Message := Format(FMT_Message,[ColorNametbDel]);
      Reply := Application.MessageBox(PChar(FMT_Message), pChar(APPLICATION_Title), BoxStyle);
      if Reply = IDYES then begin
          if dbqueries.DeleteInkRecord('ink_inv',INK_INV_ID_EDIT) Then begin
             if menuShowEmptyInk.Checked Then CheckEmptyInk:= true else CheckEmptyInk:= false;
             dbqueries.FillColorGrid(InkDataGrid,SQLQuery1,SQLTransaction1,Datasource1,CheckEmptyInk);
             FMT_Message := 'Ink: ''%s'' has been deleted from the inventory database.';
             FMT_Message := Format(FMT_Message,[ColorNametbDel]);
             mMessages.Append(FMT_Message);
             MessageDlg(APPLICATION_Title,
              FMT_Message, mtInformation,
              [mbOK], 0);
          end
          else
             MessageDlg(APPLICATION_Title,
              'Ink: ''' + ColorNametbDel + ''' cannot be deleted from the database.', mtError,
              [mbOK], 0);
      end;

[kpeters58]

You have not given us enough to form an opinion since only 1 (maybe 2) lines are really concerned with db access; maybe this will help you form an opinion as where you should go:

http://www.codeproject.com/Articles/493389/Four-ways-of-passing-data-between-layers

I also noted your desire to write "code <that> is friendly to others" and I find that your code is more verbose than need be (that's just my very personal opinion and not meant as negative criticism at all):

Code: [Select]

      Reply := Application.MessageBox(PChar(FMT_Message), pChar(APPLICATION_Title), BoxStyle);
      if Reply = IDYES then
I would shorten to

   

Code: [Select]

   if Application.MessageBox(PChar(FMT_Message), pChar(APPLICATION_Title), BoxStyle) = IDYES then
Similarly:

Code: [Select]

if menuShowEmptyInk.Checked Then CheckEmptyInk:= true else CheckEmptyInk:= false;
Could be:

Code: [Select]

CheckEmptyInk := menuShowEmptyInk.Checked;
It looks like you don't need CheckEmptyInk at all - you could just replace it in your call to FillColorgrid with 'menuShowEmptyInk.Checked' (but then I don't have all the code and could thus be wrong)


Less is *usually* more when it comes to readability and bug count.

[allanregistos]

You have not given us enough to form an opinion since only 1 (maybe 2) lines are really concerned with db access; maybe this will help you form an opinion as where you should go:

http://www.codeproject.com/Articles/493389/Four-ways-of-passing-data-between-layers

Thanks for the link.  I stated that all of my code resembles with what I provided. Sorry if that is not enough. Its all about DB access, the queries including inserts/delete are separated in a unit.  My software is all about inventory of existing raw materials such as paper rolls.  I am not really good into OOP concepts.  However, I will try to explore the TIOPF approach. Downloaded the Database Desktop but unfortunately in Ubuntu 12.04/64-bit env., it cannot find the postgresql client libraries even if I copied the libpq* in the same dir where the 'Database Desktop' was located but that another story.

I also noted your desire to write "code <that> is friendly to others" and I find that your code is more verbose than need be (that's just my very personal opinion and not meant as negative criticism at all):

Code: [Select]

      Reply := Application.MessageBox(PChar(FMT_Message), pChar(APPLICATION_Title), BoxStyle);
      if Reply = IDYES then
I would shorten to

   

Code: [Select]

   if Application.MessageBox(PChar(FMT_Message), pChar(APPLICATION_Title), BoxStyle) = IDYES then
Similarly:

Code: [Select]

if menuShowEmptyInk.Checked Then CheckEmptyInk:= true else CheckEmptyInk:= false;
Could be:

Code: [Select]

CheckEmptyInk := menuShowEmptyInk.Checked;
It looks like you don't need CheckEmptyInk at all - you could just replace it in your call to FillColorgrid with 'menuShowEmptyInk.Checked' (but then I don't have all the code and could thus be wrong)

Less is *usually* more when it comes to readability and bug count.

That helps a lot, thanks kpeters58.
-Allan

[kapibara]

Might be a far shot regarding your libpq libraries, but under Linux/Ubuntu it may be necessary to create a symbolic link, see link:

http://wiki.freepascal.org/postgres#Error:_.22Can_not_load_PostgreSQL_client_library_.22libpq.dll.22.22