Refactoring a C/AL Function - an example
December 8th, 2011
In a C/SIDE application (whose true identity shall remain unknown, to protect both the innocent and my job security ;)), I recently read this function in table 5050 Contact:
CreatePerson()
GetRMSetup;
locContact := Rec;
INIT;
“No.” := NoSeriesMgt.GetNextNo(RMSetup.”Contact Nos.”,TODAY,TRUE);
Type := Type::Person;
INSERT(TRUE);
IF locContact.Type = locContact.Type::Company THEN BEGIN
COMMIT;
CLEAR(locRole);
IF FORM.RUNMODAL(FORM::Roles,locRole) = ACTION::LookupOK THEN BEGIN
locContactRole.INIT;
locContactRole.”Contact No.” := “No.”;
locContactRole.VALIDATE(”Role Code”,locRole.Code);
locContactRole.”Related Contact No.” := locContact.”No.”;
IF locContactRole.INSERT(TRUE) THEN;
END;
END;
It contains a few patterns that I usually try to avoid. As you can see, the function contains a COMMIT statement, probably to allow the RUNMODAL following it. Furthermore, being located in a table, it work directly on Rec. The function duplicates some code present elsewhere in the system, such as the code for retrieving a number for the newly created contact.
My version of the function looks something like this:
CreatePersonNew() CreateRole := FORM.RUNMODAL(FORM::Roles, Role) = ACTION::LookupOK; Contact.Type := Contact.Type::Person; Contact.INSERT(TRUE); Contact.VALIDATE(”Company No.”, “No.”); Contact.MODIFY(TRUE); IF CreateRole THEN BEGIN ContactRole.”Contact No.” := Contact.”No.”; ContactRole.VALIDATE(”Role Code”, Role.Code); ContactRole.”Related Contact No.” := “No.”; IF NOT ContactRole.INSERT(TRUE) THEN; END;
As you can see, it’s shorter (more about this in a later blog post). The COMMIT was eliminated by getting the user input (FORM.RUNMODAL) before starting the write transaction (INSERT). It no longer does its job on Rec, but on an other variable instead - much safer. Finally, it relies on the Contact table’s OnInsert trigger to find the next available number in the number series.
Do you think my version is an improvement? Do you see flaws in my code? Please let me know! 
December 9th, 2011 at 1:04 am
Great initiative! Your code is a lot more “pretty” than the original code, but I think I spotted a few differences in their results:
1) The “Contact Company No.” is filled in your code, but not in the original code. Unless the ContactRole.OnInsert does something? (But then the difference would be, that it was only filled when creating a role, where you do it in both situations.)
2) The cursor (rec) is located on the new rec in the original code, but not in your new code. If the function is called from the Contact Card, it would be a drawback.
3) The other developer uses INIT before assigning values to the new records, which I consider best practice. Unless it has been changed in recent version, then the InitValue property is only used when actually calling the INIT trigger. So you might get different contact field values if this property is used in the contact table.
4) You call the Contact.OnModify trigger, which the old code didn’t. That means you have the “Last Modified” field updated, and a bit more code executed that doesn’t exist in the OnInsert trigger.
So whether it is an improvement or not, depends on the requirements of the users
December 9th, 2011 at 9:47 am
Thanks!
BTW, this is not part of the “High Quality Code” series that I mentioned earlier. But subject-wise, it could easily have been…
Ad 1. I need to validate Company No. to populate the company-related fields. Remember that the original code used Rec, which meant their record came prepopulated with the company details. Makes sense?
Ad 2. You’re right. It’s because I left some stuff out for clarity. The production version of my code displays the newly created records in a separate form/page.
Ad 3. My Contact record is a local variable; its fields get their InitValue (if present) as part of the implicit variable initialization, at least in my 2009 R2 environment. Avoiding that call to INIT, I guess, is another reason for minimizing variable scope (using globals only when they cannot be avoided).
Ad 4. Good point - I stand corrected. In this case, however, it didn’t break any requirements (neither functional nor performance-related).
Filling the fields required for inserting, then calling INSERT, then filling the other fields, followed by a MODIFY is a pattern that I use quite often, especially in cases like this, where the OnInsert provides me with the actual primary key value (from a number series).
December 9th, 2011 at 4:24 pm
Ad1) Yes, the original code uses Rec, but it still starts with a INIT so I don’t see how anything should get populated?
Ad3) You are right! I just tested R2, 4SP3 and even 3.10 and 2.60! Apparently you don’t need to INIT a “new” variable at all, and even a CLEAR(rec) sets the initial valueS. That’s new to me, but my training was also in 1.30 a “few” years ago
I would still consider it best practice always to INIT a variable before assigning values and RESET before setting key and filters. I do it, just to make it clear, that it doesn’t rely on anything above this piece of code.
December 12th, 2011 at 1:37 pm
Ad 1. Hmmm… You’re right. I think the original requirements may have been somewhat different. Sorry for “polluting” the sample code with that difference; I hope you’ll agree the points made (avoid using Rec, avoid COMMITs, etc.) are still valid?
Ad 3. Haha, such things happen to me all the time!
Personally, I consider it a best practice to use locals whenever possible, and use each variable for a single purpose (possibly creating e.g. multiple record variables with the same subtype). That way, there’s no real need to explicitly INIT, RESET or CLEAR the variable.
December 12th, 2011 at 2:50 pm
Ad 1) Sure