Ian Branch 127 Posted December 31, 2021 Team, Happy New Year to ALL. Given the following example procedure.. procedure TMainForm.SendAPEmails(Sender: TObject); var iRecords: SmallInt; sConsumerEmail, sMsgSubject, sMessage, sManufacturer, sModel, sESN: string; sCompanyName, sContactName, sServiceEmail, sServiceEmailName, sCCEmailAddress: string; MailSender: TELMailSMTPBaseSender; Rslt: TResponse; // const sCannot: string = #13#10 + 'Cannot send AP Emails!'; begin // if Trim(TCompanyInfo.mscStillAPMsgName) = '' then begin MessageBeep(MB_ICONERROR); TaskMessageDlg('Awaiting Parts Message Status!', 'There is no "Still AP Message Name" specified in ... the Company table!' + sCannot, mtError, [mbOK], 0); EXIT; end; // try // LTI1.ShowBalloonHint('AP Emails...', 'Processing of today''s Awaiting Parts emails is commencing.'); // // Check if TCompanyInfo.mscStillAPMsgName is present in the Messages table. Messages.Filtered := False; Messages.Filter := 'MsgName = ' + QuotedStr(TCompanyInfo.mscStillAPMsgName); Messages.Filtered := True; Messages.Open; // if Messages.eof then begin Messages.Close; MessageBeep(MB_ICONERROR); TaskMessageDlg('Awaiting Parts Message Status!', '"Still AP Message Name" ' + TCompanyInfo.mscStillAPMsgName + ' not found in the Messages table!' + sCannot, mtError, [mbOK], 0); EXIT; end; // Messages.Close; // SendAPMsg.ParamByName('BusCode').AsString := TUsersData.msuBusCode; SendAPMsg.ParamByName('iAPUpdateDays').AsInteger := TCompanyInfo.micAPUpdateDays; SendAPMsg.Open; // iRecords := SendAPMsg.RecordCount; // if iRecords = 0 then EXIT; // if iRecords = 1 then sMessage := 'There is one Awaiting Parts Update email to be sent.' else sMessage := 'There are ' + IntToNumberText(iRecords) + ' Awaiting Parts Update emails to be sent.'; // TaskMessageDlg('Awaiting Parts Status!', sMessage, mtInformation, [mbOK], 0); // while not SendAPMsg.eof do begin // sJobNo := SendAPMsg.FieldByName('jobNo').AsString; sConsumerName := Trim(SendAPMsg.FieldByName('ConsumerName').AsString); sConsumerPhone := Trim(SendAPMsg.FieldByName('ConsumerPhoneNo').AsString); sCustomerRef := SendAPMsg.FieldByName('CustomerRef').AsString; sConsumerEmail := Trim(SendAPMsg.FieldByName('ConsumerEmail').AsString); sManufacturer := Trim(SendAPMsg.FieldByName('Manufacturer').AsString); sESN := Trim(SendAPMsg.FieldByName('ESN').AsString); sModel := Trim(SendAPMsg.FieldByName('Model').AsString); // sCompanyName := Trim(SendAPMsg.FieldByName('Name').AsString); sContactName := Trim(SendAPMsg.FieldByName('SvcContactName').AsString); sServiceEmail := Trim(SendAPMsg.FieldByName('ServiceEmail').AsString); sServiceEmailName := Trim(SendAPMsg.FieldByName('ServiceEmailName').AsString); sCCEmailAddress := Trim(SendAPMsg.FieldByName('CCEmailAddress').AsString); // if sConsumerEmail <> '' then begin // // if CheckBlackListEmails(sConsumerEmail) = True then // begin // // // SendAPMsg.Next; // // // Continue; // Do not do the rest of this While loop this time. // // // end; // MailSender := TELMailSMTPClientSender.Create; // try // Copy the current EurekaLog project settings: MailSender.Options := CurrentEurekaLogOptions; // // Set up send method {$IFDEF MailTest} // MailSender := SetMailTestParams(MailSender); // {$ELSE} // // Sender if Trim(TCompanyInfo.mscServiceEmailName) = '' then MailSender.Options.OverrideUserFullName := StrTokenAt(Trim(TCompanyInfo.mscServiceEmail), '@', 1) else MailSender.Options.OverrideUserFullName := Trim(TCompanyInfo.mscServiceEmailName); // MailSender.Options.SendSMTPClientFrom := Trim(TCompanyInfo.mscServiceEmail); // MailSender.Options.SendSMTPClientTarget := sConsumerEmail; // Recipient // {$ENDIF} // MailSender.Options.SendSMTPClientAdditionalHeaders := 'CC: ' + sCCEmailAddress + sCRLF + 'X-Priority: 1' + sCRLF + 'X-MSMail-Priority: Normal' + sCRLF + 'Importance: Normal' + sCRLF; // MailSender.Options.edoUseRealName := False; // MailSender.Options.CustomizedTexts[mtSendDialog_Caption] := 'Sending Awaiting Parts Update email to ' + MailSender.Options.SendSMTPClientTarget + '...'; // // Message Subject sMsgSubject := sCompanyName + '. Awaiting Parts Update advice for Job #: ' + sJobNo + '.'; // MailSender.Options.SendSMTPClientSubject := sMsgSubject; // Subject Text // // Message Body Messages.Filtered := False; Messages.Filter := 'MsgName = ' + QuotedStr(TCompanyInfo.mscStillAPMsgName); Messages.Filtered := True; Messages.Open; // sMessage := MessagesMsgText.AsString; // Messages.Close; // // sMessage variables manually set as Jobtickets is not open att. sMessage := ReplaceText(sMessage, '%JobNo%', sJobNo); sMessage := ReplaceText(sMessage, '%CustomerRef%', sCustomerRef); sMessage := ReplaceText(sMessage, '%CompanyName%', sCompanyName); sMessage := ReplaceText(sMessage, '%ContactName%', TCompanyInfo.mscCoyContactName); sMessage := ReplaceText(sMessage, '%ConsumerName%', sConsumerName); sMessage := ReplaceText(sMessage, '%SvcContactName%', TCompanyInfo.mscCoySvcContactName); sMessage := ReplaceText(sMessage, '%Position%', TCompanyInfo.mscCoySvcPosition); sMessage := ReplaceText(sMessage, '%Manufacturer%', sManufacturer); sMessage := ReplaceText(sMessage, '%IMEI%', sESN); sMessage := ReplaceText(sMessage, '%Model%', sModel); // MailSender.Options.SendSMTPClientMessage := sMessage; // MailSender.Options.SendSMTPClientAppendLogs := False; MailSender.Options.SendSMTPClientUseRealEMail := False; // // For visual dialog during sending: MailSender.ProgressIndicator := ProgressIndicator; FSendProgress := TSendDialogMSClassic.Create(Handle, MailSender.Options); // try // Actual send Rslt := MailSender.SendMessage; // finally FreeAndNil(FSendProgress); end; // if Failed(Ord(Rslt.SendResult)) then begin // LTD1.ExpandedText := Trim(Rslt.ErrorMessage); LTD1.Text := 'Oooops! Sending Awaiting Parts Update email for JT#' + sJobNo + ' failed.' + sCRLF + 'Click the expand button below to view the full error message.' + sCRLF + 'Awaiting Parts Update email sending will now exit.'; LTD1.Execute(Self.Handle); // SendAPMsg.Close; // EmailsLog.Open; EmailsLog.AppendRecord([now, 'AP Update', 'Sending Failed - Customer Awaiting Parts Update email to ' + sConsumerName + ' for Job #: ' + sJobNo + '.', MailSender.Options.SendSMTPClientTarget, TUsersData.msuUserID, StrToInt(sJobNo)]); EmailsLog.Close; // // Exit; // end else begin // {$IF Defined(ELogging) or Defined(Codesite)} LogMessage('Customer Awaiting Parts Update email sent to ' + sConsumerName + ' for Job #: ' + sJobNo + '.'); {$IFEND} // with APJobTickets do begin Open; Edit; FieldByName('DateSent').AsDateTime := now; Post; Close; end; // EmailsLog.Open; EmailsLog.AppendRecord([now, 'AP Update', 'Customer Awaiting Parts Update email sent to ' + sConsumerName + ' for Job #: ' + sJobNo + '.', MailSender.Options.SendSMTPClientTarget, TUsersData.msuUserID, StrToInt(sJobNo)]); EmailsLog.Close; // end; // SendAPMsg.Next; // finally FreeAndNil(MailSender); end; // /// ///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// // end else SendAPMsg.Next; // end; // End While. // LTI1.ShowBalloonHint('AP Emails...', 'Processing of today''s Awaiting Parts emails has completed.'); // finally // SendAPMsg.Close; // end; // end; How do I move/translate that over to a formless Unit please? I have several Print routines similar to this in the Main Form that I would like to put into their own Unit rather than in the Main Form. This is not something I have done before. Regards & TIA, Ian Share this post Link to post
Rollo62 536 Posted December 31, 2021 (edited) TLDR; I think you mixed too many stuff in one method. I would separate this into different domains, each in a unit, e.g. like UMailSend.pas, USysInfo.pas, and the caller I usually encapsule separate functions into classes, with static functions/methods. This behaves same like functions or methods, but keep them under a "namespace" of the class, which could be extended if needed. type TMail = class class function Send( AReceiver, AMessage : String, ...); static; end; Edited December 31, 2021 by Rollo62 2 Share this post Link to post
David Heffernan 2345 Posted December 31, 2021 Get Martin Fowler's book on refactoring. 3 Share this post Link to post
Ian Branch 127 Posted December 31, 2021 Rollo62 - One step at a time. 😉 David - Done. Share this post Link to post
Mike Torrettinni 198 Posted December 31, 2021 18 hours ago, Ian Branch said: procedure TMainForm.SendAPEmails(Sender: TObject); I refactored 100s of methods like this, from within forms, in past few years, and the first thing I do is to make it independent of form and controls. So, remove form reference to become: procedure SendAPEmails(parameters...); also move the declaration out of the form, but keep in the same unit. Now make it work without reference to the form or controls. You will probably need to set parameters to pass values to assign to controls. Once it works as a standalone method, move it to new unit. You will see what uses clause you need to define to work. Perhaps you will need to move a few more methods that are used just by this method. Do the same for other print routines. Once you move a few or all of them, then it will be easier to see what patterns of code are repeated so you can refactor to smaller methods. I found this was best for my case, trying to refactor in the form unit with lots of other code was quickly becoming unmanageable mess. But if you have these methods in smaller unit(s) the refactoring is much easier and more obvious. Share this post Link to post
Ian Branch 127 Posted December 31, 2021 Thanks Mike, Your thoughts/insights appreciated. Ian Share this post Link to post
Ian Branch 127 Posted January 1, 2022 Hi All, I know you are all just dying to know how I am progressing with this.. ;-) Following Mike's suggestions, I firstly made it work without being a TMainForm procedure. This meant referencing the MainForm components in the procedure with the TMainForm. prefix. I then Copied the entire routine and all the Uses, Var and Const clauses over as well into a new Unit just to make sure there were no initial issues. Then there were the components on the MainForm that were being referenced. e.g. tables, queries, dialogs, etc, which needed to be created. I could have left them in the MainForm and just left the TMainForm. references but I don't believe that is the right way to go. I am open to advice/opinions on this. I created the tables and queries in code, all good. I now have to create the remaining MainForm components, dialogs, etc, in code and it should all be good. I have been able to eliminate, through a process of commenting out and seeing what breaks, a significant amount of uses clause entries, as well as some var and const entries. I suspect some of the uses clause entries may have to return as I create the dialogs, etc in code. I have run up against an issue with a third party library which I am pursuing att, all works perfectly in the Form but not in the Unit. The 3rd Party developer is looking into it. Like I said in my first post, this is something new for me. As I work the process I can see other opportunities in other Apps/modules as well however I need to get this right first. :-) I will keep this thread updated as I progress. This may not interest the experienced amongst you but there may be some value to those new to Delphi or like me, new to doing this specifically. At the end of it all I will post the final Unit, as an attachment, for anybody's reference. Regards, Ian Share this post Link to post
Mike Torrettinni 198 Posted January 1, 2022 17 minutes ago, Ian Branch said: This meant referencing the MainForm components in the procedure with the TMainForm. prefix. I then Copied the entire routine and all the Uses, Var and Const clauses over as well into a new Unit just to make sure there were no initial issues. Then there were the components on the MainForm that were being referenced. e.g. tables, queries, dialogs, etc, which needed to be created. I could have left them in the MainForm and just left the TMainForm. references but I don't believe that is the right way to go. I am open to advice/opinions on this. I created the tables and queries in code, all good. I now have to create the remaining MainForm components, dialogs, etc, in code and it should all be good. Can you show very simple example what you did here, for 1 control what was before and how it is now? I don't understand why you are recreating controls... the originals were part of MainForm, right? I just want to make sure my suggestion didn't lead you down the wrong path. Share this post Link to post
Ian Branch 127 Posted January 1, 2022 Hi Mike, Sure. var APJobTickets: TnlhTable; SendAPMsg: TnlhQuery; dsSendAPMsg: TDataSource; procedure CreateAPDataComponents; begin // SendAPMsg := TnlhQuery.Create(nil); // with SendAPMsg do begin // DataBaseName := dmC.DBC1.DatabaseName; SessionName := dmC.DBC1.SessionName; SQL.Text := 'select s.*, j.JobNo, j.CustomerRef, j.ConsumerName, j.ConsumerPhoneNo, j.ConsumerEmail, j.Manufacturer, j.Model, j.ESN, j.ConsumerName, c.Name,'; SQL.Text := SQL.Text + 'c.ContactName, c.ServiceEmail, c.ServiceEmailName, c.CCEmailAddress, c.SvcContactName from APJobTickets s, JobTickets j, Company c'; SQL.Text := SQL.Text + 'where (j.buscode = :BusCode) and (j.JobNo = s.JobNo) and (c.BusCode = j.BusCode) and ((Current_Date - Cast(s.DateSent as Date)) >= :iAPUpdateDays) and (Trim(BOTH '' '' from j.ConsumerEmail) <> '''')'; SQL.Text := SQL.Text + 'order by s.JobNo'; ReadOnly := True; // end; // dsSendAPMsg := TDataSource.Create(nil); // with dsSendAPMsg do begin // DataSet := SendAPMsg; AutoEdit := False; // end; // APJobTickets := TnlhTable.Create(nil); // with APJobTickets do begin // DataBaseName := dmC.DBC1.DatabaseName; SessionName := dmC.DBC1.SessionName; ; TableName := 'APJobTikets'; AutoCalcFields := True; IndexName := 'JobNo'; MasterSource := dsSendAPMsg; MasterFields := 'JobNo'; NullEmptyStrings := True; RTrimMemos := True; LTrimStrings := True; // end; // end; From the above, SendAPMsg, dsSendAPMsg and APJobTickets are used as normal within the SendAPEmails procedure. The components I am creating were only on the MainForm for the SendAPEmails procedure I am now moving to a Unit. Regards, Ian Share this post Link to post
Anders Melander 1784 Posted January 1, 2022 Not related to your specific question but... On 12/31/2021 at 5:16 AM, Ian Branch said: MessageBeep(MB_ICONERROR); Get rid of that. TaskMessageDlg already plays the system sound associated with the message type. On 12/31/2021 at 5:16 AM, Ian Branch said: const sCannot: string = #13#10 + 'Cannot send AP Emails!'; Does sCannot really need to be a typed const? As far as I remember it's better type use untyped string consts since these will end up in R/O memory. Also you use #13#10 here but sCRLF everywhere else. Personally I prefer #13#10 (or just #13 when that is appropriate) since it's more explicit. On 12/31/2021 at 5:16 AM, Ian Branch said: MailSender := TELMailSMTPClientSender.Create; try ... finally FreeAndNil(MailSender); end; Since MailSender is a local var and only used inside the try...except block, there's no point in using FreeAndNil() here. Just use Free instead. If you only use FreeAndNil when there's a reason to then it will communicate to the reader that here's something to be aware of; The var needs to be nil when it doesn't reference a valid instance. On 12/31/2021 at 5:16 AM, Ian Branch said: with APJobTickets do ... Since you're already refactoring all this, get rid of the "with" statements - and forget you ever learned about it. Share this post Link to post
Ian Branch 127 Posted January 1, 2022 (edited) Hi Mike, Thank you for your feedback. Rather than quoting you each time I will simply respond in order... MessageBeep/TaskDialog - I was not aware of that. Tks. sCannot - Sorry, I don't understand. sCRLF v #13#10 - I am in the process of changing sCRLFs back to #13#10. MailSender - Is declared as a Unit variable and will be created/destroyed by other emailing routines. Is your comment around this still applicable? with APJobTickets - I am aware of the concerns expressed around using with. I figure that as their usage here is in a controlled single action, not nested, there is a word for it but I can't recall it (atomic?), then there should be minimal risk, but improve readability. I think this 'discussion' will last as long as 'with' is in the language. 🙂 Ian Edited January 1, 2022 by Ian Branch Share this post Link to post
Anders Melander 1784 Posted January 1, 2022 2 minutes ago, Ian Branch said: sCannot - Sorry, I don't understand. Instead of this: const sCannot: string = #13#10 + 'Cannot send AP Emails!'; do this: const sCannot = #13#10 + 'Cannot send AP Emails!'; and then read: https://docwiki.embarcadero.com/RADStudio/Sydney/en/Declared_Constants#Typed_Constants As I read the documentation, if you make the const typed then the compiler cannot concatenate the strings at compile time but will have to do it at run time. Probably because typed consts can be modified at runtime if you have that option enabled. 2 minutes ago, Ian Branch said: MailSender - Is declared as a Unit variable and will be created/destroyed by other emailing routines. In your example it's a variable local to the method. 4 minutes ago, Ian Branch said: I am aware of the concerns expressed around using with. I figure that as their usage here is in a controlled single action, not nested, there is a word for it but I can't recall it, atomic? then there should be minimal risk, but improve readability. I think this 'discussion' will last as long as 'with' is in the language. 🙂 I'm sure there are also still people who argue that it's safe to run with scissors as long as <insert excuse here>. No seriously, stop using "with". It doesn't make you code more readable. On the contrary. It's impossible for someone else (i.e. that unfortunate person who will maintain the code after the author has moved on to greener pastures) to see if an identifier references a "with'ed" member or something higher up in the scope. Also the debug inspector can't cope with it. You might think that in your case it's safe because <insert excuse here> but I could identify several things that could go wrong in your code that you apparently haven't thought of. Share this post Link to post
Ian Branch 127 Posted January 1, 2022 Hi Mike, sCannot - Interesting. I will look into this. MailSender - Post posting that I moved it to a Unit Var rather than a local. 'With' - 4 minutes ago, Anders Melander said: You might think that in your case it's safe because <insert excuse here> but I could identify several things that could go wrong in your code that you apparently haven't thought of. I am more than happy to be educated and shown the error of my ways. 9 minutes ago, Anders Melander said: It doesn't make you code more readable. On the contrary. I think it is more subjective if this is more readable.. procedure CreateAPDataComponents; begin // SendAPMsg := TnlhQuery.Create(nil); // SendAPMsg.DataBaseName := dmC.DBC1.DatabaseName; SendAPMsg.SessionName := dmC.DBC1.SessionName; SendAPMsg.SQL.Text := 'select s.*, j.JobNo, j.CustomerRef, j.ConsumerName, j.ConsumerPhoneNo, j.ConsumerEmail, j.Manufacturer, j.Model, j.ESN, j.ConsumerName, c.Name,'; SendAPMsg.SQL.Text := SendAPMsg.SQL.Text + 'c.ContactName, c.ServiceEmail, c.ServiceEmailName, c.CCEmailAddress, c.SvcContactName from APJobTickets s, JobTickets j, Company c'; SendAPMsg.SQL.Text := SendAPMsg.SQL.Text + 'where (j.buscode = :BusCode) and (j.JobNo = s.JobNo) and (c.BusCode = j.BusCode) and ((Current_Date - Cast(s.DateSent as Date)) >= :iAPUpdateDays) and (Trim(BOTH '' '' from j.ConsumerEmail) <> '''')'; SendAPMsg.SQL.Text := SendAPMsg.SQL.Text + 'order by s.JobNo'; SendAPMsg.ReadOnly := True; // dsSendAPMsg := TDataSource.Create(nil); // dsSendAPMsg.DataSet := SendAPMsg; dsSendAPMsg.AutoEdit := False; // APJobTickets := TnlhTable.Create(nil); // APJobTickets.DataBaseName := dmC.DBC1.DatabaseName; APJobTickets.SessionName := dmC.DBC1.SessionName; APJobTickets.TableName := 'APJobTikets'; APJobTickets.AutoCalcFields := True; APJobTickets.IndexName := 'JobNo'; APJobTickets.MasterSource := dsSendAPMsg; APJobTickets.MasterFields := 'JobNo'; APJobTickets.NullEmptyStrings := True; APJobTickets.RTrimMemos := True; APJobTickets.LTrimStrings := True; // end; We can agree to disagree on readability. 🙂 Ian Share this post Link to post
Mike Torrettinni 198 Posted January 2, 2022 1 hour ago, Ian Branch said: var APJobTickets: TnlhTable; SendAPMsg: TnlhQuery; dsSendAPMsg: TDataSource; I assume these are global variables accessed from MainForm, right? If only used in MainForm, then don't use them as globals but define them as MainForm fields and pass them as parameters to the method: MainForm: TMainForm ... private fAPJobtickets fSendAPMsg fdsSendAPMSG // then on some action (button click, form create...) create them like this: CreateApDataComponents(fAPJobtickets, fSendAPMsg, fdsSendAPMSG); there are multiple ways to implement this, but this is simple example that will then allow you to expand, change.... try to avoid global vars. Also, make sure the new unit doesn't use MainForm unit - avoid circular reference, if possible. Share this post Link to post
Ian Branch 127 Posted January 2, 2022 49 minutes ago, Mike Torrettinni said: I assume these are global variables accessed from MainForm, right? In the new structure - not used by the MainForm at all. Once I have all this sorted there will be 5 or 6 different emailing actions in the Unit called by a single SendEmails procedure in the Unit. The SendEmails Procedure will be called from the MainForm. Share this post Link to post
Mike Torrettinni 198 Posted January 2, 2022 (edited) 40 minutes ago, Ian Branch said: In the new structure Then you can make them as fields of this new structure. If these variables are local to unit (defined in implementation) than leave them, if you really need to, but with such a small number of vars this is good opportunity to practice removing/converting global vars. Edited January 2, 2022 by Mike Torrettinni Share this post Link to post
Stano 143 Posted January 2, 2022 (edited) Quote I think it is more subjective if this is more readable. I do not agree. The reasons were written by Anders Meland. Read them a few times. This section is not designed correctly. SendAPMsg.SQL.Text := 'select s.*, j.JobNo, j.CustomerRef, j.ConsumerName, j.ConsumerPhoneNo, j.ConsumerEmail, j.Manufacturer, j.Model, j.ESN, j.ConsumerName, c.Name,'; SendAPMsg.SQL.Text := SendAPMsg.SQL.Text + 'c.ContactName, c.ServiceEmail, c.ServiceEmailName, c.CCEmailAddress, c.SvcContactName from APJobTickets s, JobTickets j, Company c'; SendAPMsg.SQL.Text := SendAPMsg.SQL.Text + 'where (j.buscode = :BusCode) and (j.JobNo = s.JobNo) and (c.BusCode = j.BusCode) and ((Current_Date - Cast(s.DateSent as Date)) >= :iAPUpdateDays) and (Trim(BOTH '' '' from j.ConsumerEmail) <> '''')'; SendAPMsg.SQL.Text := SendAPMsg.SQL.Text + 'order by s.JobNo'; Be aware that after each event SendAPMsg.SQL.Text := SendAPMsg.SQL.Text + something the correctness of SQL.Text is always evaluated. Compose the Text correctly first and assign it entirely to SendAPMsg.SQL.Text. If you do it right, the whole SQL.Text will be nicely readable. Now it's a cluster of some letters. Remember to have the right line length. They must be fully visible. I have 120 characters Edited January 2, 2022 by Stano Share this post Link to post
Ian Branch 127 Posted January 2, 2022 Hi Stano, That SQL has already been re-done to a single string variable that is then assigned to .SQL.Text. But, I do take your point about the evaluation aspect for future reference. Ian Share this post Link to post
David Schwartz 426 Posted January 2, 2022 (edited) I'm pretty sure you are struggling here because you don't have a clear idea of the notions of "encapsulation" when it comes to OOP. Or SOLID principles. Your GUI stuff should not need to be replaced with components created at run-time because THEY BELONG TO THE FORM and you are apparently unable to access them directly after removing this method from the form's class. So your solution is to back-door the form to override the encapsulation. You also said that after removing it you replaced all of the references to components on the form with T<theformname>, which is wrong. You want the INSTANCE variable name, not the TYPE. (Also, the TYPE is NOT the UNIT, which is I think what you're actually thinking of here.) These components are NOT static members of the form class (or the unit), they are MEMBERS of every INSTANCE of the FORM. So one way to solve this dilemma is to pass in a form parameter to the method that it needs to interact with, rather than give this unrelated function carte blanche to create stuff inside of the form's object! The problem with this approach is that then the email method is tightly coupled to this particular form. It also creates a circular reference between the UNITS. You could solve that by defining an Interface (in another unit) so the email method could be used with any form. But it's really not the job of the email method to update the UI anyway! It's job is to manage email, right? The FORM is responsible for interacting with the user. So you really have no business creating a way for the email method to even touch the UI. So scratch this one off as well. You need to realize that conceptually speaking, the email method could be stuck off on server somewhere and access as a REST service. In that case, the separation of responsibility becomes strikingly visible -- it's impossible for the email method to even reach the form, let alone update the UI. These are all classic symptoms that show up when someone doesn't get the gist of OOP "encapsulation" yet -- because your first inclination is to figure out ways around the encapsulation barriers, rather than thinking of how to restructure the interfaces to get you the results you desire. First, get the references to the UI OUT OF THE MAIL FUNCTION!!!! The UI should be managed EXCLUSIVELY by the FORM. If you want the function to update the UI, then pass in callback functions, or process things in a loop where you first do a setup, then call it for each update needed until it's done, then call a completion method. Read up on SOLID principles, especially SEPARATION OF RESPONSIBILITY. Better yet, pretend that this email method is on another server and all you can do is access it via a REST interface. That will get you the separation you need. It'll feel like you're standing on your head at first, but at least that's a sign that you're moving in the right direction. And if you find you need to split it into 3 or 4 different methods, so be it. And ... it probably belongs in its own class anyway that gets instantiated by the FORM to provide access to some email services. That is, it should live in a library and know absolutely nothing about who's using it. Edited January 2, 2022 by David Schwartz Share this post Link to post
Attila Kovacs 629 Posted January 2, 2022 13 minutes ago, David Schwartz said: If you want the function to update the UI, then pass in callback functions, or process things in a loop where you first do a setup, then call it for each update needed until it's done, then call a completion method. Events could be useful. OnProgress OnError OnQueryCancelled etc... 14 minutes ago, David Schwartz said: Better yet, pretend that this email method is on another server and all you can do is access it via a REST interface. Or fire up a brand new empty project and both projects should be able to use the same unit. But only that one. But, as far as I can see, this is not Ian's main goal this time. He is learning different things first. Share this post Link to post
Ian Branch 127 Posted January 3, 2022 22 minutes ago, David Schwartz said: I'm pretty sure you are struggling here because you don't have a clear idea of the notions of "encapsulation" when it comes to OOP. Or SOLID principles. Hi David, That would be a reasonable/accurate statement. I can access the MainForm UI components from the Unit but I was hoping to make the Unit totally self-contained and NOT have any of them on the MainForm. For other reasons, not pertinent to this discussion, my intent can no longer be pursued. 😞 ATT I will be sticking with all the routines/procedures/UI in the MainForm. My thanks to all that have provided suggestions, instruction and guidance. Regards, Ian Share this post Link to post
David Schwartz 426 Posted January 3, 2022 (edited) 4 hours ago, Ian Branch said: Hi David, That would be a reasonable/accurate statement. I can access the MainForm UI components from the Unit but I was hoping to make the Unit totally self-contained and NOT have any of them on the MainForm. For other reasons, not pertinent to this discussion, my intent can no longer be pursued. 😞 ATT I will be sticking with all the routines/procedures/UI in the MainForm. My thanks to all that have provided suggestions, instruction and guidance. Regards, Ian Actually, I would encourage you to continue. It MUST be pursued! Because this is really the only way to get your mind to "flip" so you see things from an OOP perspective. It took me about 6 months of banging my head against these virtual walls and coding myself into dead-end alleys until one day I looked at the code and something flipped around and suddenly I saw it correctly. You've got to stick with it until that shift happens for you. You don't see it now. Once you do, it's game over for the "old" way of thinking. This is a perfect example to work with. Just keep banging away at it until you wrestle it to the ground, and step back and suddenly see what's required. Edited January 3, 2022 by David Schwartz 1 Share this post Link to post
Fr0sT.Brutal 900 Posted January 10, 2022 (edited) On 1/2/2022 at 1:18 AM, Anders Melander said: Also you use #13#10 here but sCRLF everywhere else. Personally I prefer #13#10 (or just #13 when that is appropriate) since it's more explicit. Hmm, named constant with clear Carriage-Return&Line-Feed name is less explicit than a magic constant literal? I disagree here. The only advantage of literal is highlight in editor. On 1/2/2022 at 1:18 AM, Anders Melander said: Since MailSender is a local var and only used inside the try...except block, there's no point in using FreeAndNil() here. Just use Free instead. This is OK for unification. On 1/2/2022 at 1:32 AM, Ian Branch said: sCRLF v #13#10 - I am in the process of changing sCRLFs back to #13#10. I'd advice not doing this. Side note: it took me some time to realize the difference between "Line break" (aka "New line" or "End of line" sign) and "CRLF" which are the same on Windows but not on other OS's. EOL is OS-dependent and CRLF is constant. Edited January 10, 2022 by Fr0sT.Brutal Share this post Link to post
Anders Melander 1784 Posted January 10, 2022 14 minutes ago, Fr0sT.Brutal said: This is OK for unification. You misspelled Obfuscation. Share this post Link to post
corneliusdavid 214 Posted January 10, 2022 7 hours ago, Fr0sT.Brutal said: it took me some time to realize the difference between "Line break" (aka "New line" or "End of line" sign) and "CRLF" which are the same on Windows but not on other OS's. EOL is OS-dependent and CRLF is constant. This is why I use the constant sLineBreak as defined in the System unit. Share this post Link to post
![Delphi-PRAXiS [en]](https://en.delphipraxis.net/uploads/monthly_2018_12/logo.png.be76d93fcd709295cb24de51900e5888.png){kind=logo}
