How Good Can AL Code Get? — A Live ISO 5055 Review

Hinzugefügt: 2026-05-09

[00:00:15]Okay, I think no need to switch back and forth. Forgot to open up the start screen, right? Uh I hope this is working. Let me just double check this real quick.

[00:00:33]See that the live stream picked up. That looks good. All right. Closing again to not reduce artifacts. All right. Hello.

[00:00:40]Welcome to today's uh stream.

[00:00:44]Um today I won't be playing around with any claw code or AI or anything. Um so I hope you don't mind. Today's topic is uh code review and quality and everything. So uh brace yourself for some deep discussions. Uh one thing from the beginning now uh I don't think anybody's watching live yet but uh I hope the music works now. I got some comments on recent videos that the music was too loud uh or that I should remove it completely, but I feel like without anything without any music, it's might be a little silent at times, especially for like AI videos where I need to or where I don't have something to talk all the time. So um I'll keep the music but I try to keep it silent. So if somebody finds the music uh too loud still let me know then I will turn it down further and I hope everything else works.

[00:01:57]All right. So this is an extension.

[00:02:02]It has my name as a publisher but it's uh mainly developed and the main idea was by spare brand licensing now spare ideas and the head behind that is uh my fellow MVP Jeremy and I took this as an example to walk you through how I do code reviews. Now, I want to be absolutely transparent with this. The code review service that this is all about is a paid service of mine, but I still found it interesting or think it's interesting to see uh a little bit behind the curtains like what roots do I do? What how is the how does the tooling look like that I use? um and maybe some ideas that you can also adapt for yourself and for your own code reviews in your company or whatever uh you do to improve or drive quality.

[00:03:04]All right. So, just one thing at the at the start, I've also linked this into the description.

[00:03:13]This is not necessary.

[00:03:17]Um, so my service page on my company site basically it describes all of this in in in a little bit of more detail. And I've also listed and this is what we will be looking at today a little bit all the different rules that this review that I do runs against. And it basically contains two different uh areas. one is uh basically derived from ISO 5055 which is um a software measurement or review standard uh language independent mostly and uh this is versioned this is uh nothing new there are many uh like versions already and like many roots in in this um gone and this has 119 rules. Now this is annoying.

[00:04:29]Um I have evaluated all of them in detail and not all of them are fully applicable. We'll see that in a minute.

[00:04:39]And then on top of this I've or I am maintaining a uh I call a quality framework but a set of rules.

[00:04:49]Not sure where this link links to.

[00:04:52]Okay. A set of rules on top which are custom to AL development and things that I find uh most valuable for or very important for AI AL development. This same link again. Okay. So this keeping or this set um let's jump into the code.

[00:05:18]So the first uh starting point maybe is a a quality assessment I also offer and this is actually a free service you can you can leverage on my website as well.

[00:05:30]Um, let's see.

[00:05:33]This doesn't work. Reveal, which is basically a like a a review just to give you a little bit of a scoring like how many validations or violations you had, potential findings, how many rules were evaluated, just like a quick scoreboard, so to speak, which runs my analyzer in the background. A little bit of explanation.

[00:05:56]And this is again this is a free offering.

[00:05:59]Um but this is not what I like done with the promotion promoting things. Why does this keep coming up?

[00:06:09]Um now let's look at some rules in in detail. Uh one thing I wanted to show you is what happens to this or how the code looks like with the default analyzers.

[00:06:24]So reg normally you would probably have a configuration similar to this. Let me just copy this. Let's disable this one and activate for apps source cops the code cop the UI cop and the app source cop just like this.

[00:06:42]And let's remove the active five filter.

[00:06:44]So this are those are the the warnings or the diagnostics that we get from this extension.

[00:06:54]Right? So let's switch back to the problems view. We have internal visibility too which is correct.

[00:07:02]Um declared by reference but never used and a few text constants that are not translatable objects. And that's it.

[00:07:14]spelling also complains but uh beyond that there are no further issues in this um in this repository. I'm not sure if the if the rule set is configured for patented extension cop to run in parallel that might throw some false positives here.

[00:07:38]Okay, now we have some some uh audience here live with me. Again, my question, I've asked it in the beginning. Is the music in the background disturbing or is it just right or does does it need adjustment?

[00:07:55]Okay, continuing. Waiting for the answer, but continuing. Uh, pretend extension cop doesn't change the picture. Now if I would add the AL cops like this well test automation we don't need but everything else then the picture changes a little bit we have some more warnings from the or more diagnostics from the community re code cups for example modify without a parameter for example Uh let's close this. Um iso datetime date for example.

[00:08:44]Let's see commit needs a justifying explanation. You you if you're familiar with the lintter cup project you probably recognize many of them. Others might be new. There is also co cognitive complexity here involved.

[00:08:58]with with some uh some warnings. Let's see if we have some more. Public docu uh public procedures must have documentation tool tip must start with specifies.

[00:09:10]There are many things in here that could still be improved.

[00:09:14]So this would probably be also a project on its own to also address all of those or many of those uh things that are that are complained about here from those.

[00:09:28]And now let's switch back to to the uh now I need to enter this because I think it's VS Code complaining about or trying to get access to my uh SSH key here for GitHub.

[00:09:44]One second.

[00:09:50]All right. Um, let's go back to the settings and paste my review analyzer here. So, what I've built is a proprietary analyzer. I won't share the source code of, but I can show you a little bit of what it what it complains about or what it shows us. Um so and and the the diagnostics here are different now and we have about 500 of them. So let's see a little bit what this reports.

[00:10:24]For example, it says that the permission property regions modify declares more access than the object requires. Let's see.

[00:10:45]Well, this is to begin with it's an obsolete object. So there is no body in here anymore. So it's it's actually correct. This is what this this analyzer um tries to find things like this. Now in my review process I would flag this as a either false positive or a justified exception because uh while the warning is correct it doesn't mean anything because there is no code in here anymore and probably should be removed but doesn't need to because the entire object will be removed soon. So but there is more to this. So we see that this is a CWE 732. So this means this is mapped to the ISO uh 5055 standard. So if we what was it 732 we go onto this one second incorrect permission assignment permission sets grant brower access then intended expose sensitive data or operations to unauthorized users. And this is the general explanation of that CWE. So if we click the link, we can actually see the uh the definition here.

[00:12:07]Um and this is mainly about security. I believe the rule is also in the security section. Yeah.

[00:12:15]So I mean this is about indirect permissions. Um not sure if if the uh there is really a security risk here, but I still find that defining more permissions than actually needed should be should be fixed. It's not that clean. That's why I decided to map this on on here.

[00:12:42]Let's see if we can Okay, let's go to another rule.

[00:12:52]for example.

[00:12:59]Well, that's a very specific one for performance or patterns in this case.

[00:13:04]Let's see for um EAT-10 missing expl explicit read isolation on record read. So read reads without explicit read isolation defaults to escalate update locks invisibly causing concurrency degradation under load.

[00:13:28]Basically just very simple we do a find operation. There is no read isolation.

[00:13:35]It also finds a few other things.

[00:13:38]Um we we are also missing load fields and we do not have a set current key.

[00:13:43]But this one's also interesting because it's something I probably want to revisit.

[00:13:57]I'm not sure why this doesn't show up.

[00:14:02]Maybe I need to update my website a little bit.

[00:14:11]But in general, ISO 5055 says that um we shouldn't rely on properties that are not guaranteed. And I didn't find any documentation I believe about the fact that when we do not specify the the current key that the primary key is being used.

[00:14:33]Um again this might be something that might be a stretch to assume this or to to require this. That's why this rule is uh also under review. I think I will update this and remove this because it doesn't make that much sense.

[00:14:50]Um let's see some more rules without redesolation. You can see there is a lot of things going on. The 1083 is very very prominent and you can see that everything's green here. Um, and I think this is this might be one I wanted to keep this to the end a little bit more, but I guess uh we we'll look at it now.

[00:15:15]1083.

[00:15:17]See, data access from outside expected data manager.

[00:15:27]Um, extension code is reading with or writing data in ways that bypass the standard BC table access model and circumvent business logic and logging.

[00:15:38]Let's look at the definition.

[00:15:41]Um, the product is intended to manage data access through a particular data manager component such as relational or nonSQL databases but contains code that performs data access operations without using that component. And now I've been going through all of those rules in detail at AI explained them to me. Uh there is also more documentation by the way. If we look at this link, it doesn't have direct uh links to the individual uh CWES since it's just a big PD PDF.

[00:16:16]But if we search in here, why is 1083 not in here?

[00:16:31]reliability. Do I have this mapped wrong?

[00:16:49]Let's see. There are only four categories and security I think is not one of them.

[00:17:02]Interesting. So those are regularly the the PDFs to explain the entire definition of all of this. So this might have a different different uh numbering.

[00:17:17]Let's search for data manager data access control from outside the data manager. There we go. So this is a different number mechanism but in general it contains a very detailed description of of all of this stuff.

[00:17:36]And here is a more detailed explanation of of what's going on. But essentially for this particular rule what I have mapped this to is um accessing or writing data by direct assignment and like circumventing the logic that is or might be defined on the table on how to control data access which means in particular or in in in detail that there are two different uh diagnostics on the same rule. rule. Basically, first of all that the order of field assignment and operation has to follow the standard of how pages interact with tables.

[00:18:24]Meaning we first run in it. We then validate the primary key fields and then we call insert true which is exactly what pages do. And then we we do this the same for all the remaining fields calling on validate and then doing a modify true which is what I've considered or what I consider the the proper way of assigning and and setting data. Now there might be exceptions of course every rule has its exceptions. I think those are fine but those have to be documented. This is how I usually do this. Whenever there is a clear reason not to call validate it should be documented correctly. But essentially um this is this rule. So it it throws on everything that is not not not compliant. That's why we have so many of those warnings because it it flags every single field access which doesn't fall in this order. So if we run this can see that this is just complaining about primary key field before insert.

[00:19:40]So just need to check what's the primary key. It's just the entry ID.

[00:19:53]Oh, it falls. Okay, there might be a false positive here.

[00:19:59]Let's also convert those real quick.

[00:20:09]You can see now um it's going the the warnings disappear because now we have the entry number before the for before the inert and then here we have we need to modify still now I already find more warnings that I probably need to adapt.

[00:20:32]We set values to a parameter or to through a variable that is local and not by reference and we do not modify them after we do not pass them further or we do pass them further down. So there might be modified down below but this would be pattern I would probably also add uh just for visibility that you set fields but you maybe forgot the modify.

[00:20:59]Um, let's see.

[00:21:03]Yes. 77 778.

[00:21:07]Let's go back to the rule list.

[00:21:10]778. Insufficient logging, security, critical events, permission escalation, bulk, data export, financial posting not captured in telemetry, leaving no audit trail. Now, telemetry here needs an update because I've uh I've changed the rule a little bit recently.

[00:21:29]But in general we say that um we we should care about logging and this is account data as data classification which triggers this rule saying well this seems to be important you might want to consider capturing it in change lock and the rule checks whether you have one of those data classifications set to to other than basically system metadata. I believe we have account data, customer content.

[00:22:10]Yeah, basically anything other than system metadata is worth logging especially the data privacy like the identifiable information uh should be should be locked and it checks whether your install code your install code unit adds those um tables and fields to the change log automatically.

[00:22:36]Now whether the customer has change lock activated or not is a different story.

[00:22:40]Um but having it configured correctly like having the extension configure itself correctly for change lock what it recommends I think uh is is not a bad bad thing.

[00:22:57]Now let's see isolated storage set without corresponding delete in the same scope.

[00:23:06]Now this this checks whether there is a delete procedure in the uh in that code unit because I assume that there is a ISO store manager like we have here and it just scans whether there is like everything implemented that comes with the isolated storage. We can also check the CWE explanation for this incomplete cleanup. So this is something ISO cares about.

[00:23:37]The product does not properly clean up and remove temporary and supporting resources after they have been used.

[00:23:45]Again, I had to uh adapt this a little bit of what patterns we do have in AL.

[00:23:50]But I think leaving isolated storage traces in the in the ISO storage is not not necessarily good practice. So cleaning up would would be nice. Now I don't I know we do not have something like uh on uninstall extension or something and I'm also not sure if the cleaning if you do uninstall with removal of data if that takes care of isolated storage and things. So once we would get an event like this, I would probably add this uh this coverage to this rule. But in general, we we should do what we can do and I think isolated storage offering a delete uh procedure in this code unit would be nice.

[00:24:40]There are more more warnings here related to isolated storage.

[00:24:46]Let's see.

[00:24:50]Isolated story set without confirmed descript encryption.

[00:24:55]This is custom rule.

[00:25:00]I'm not sure if this one needs it or not.

[00:25:04]What is this even? This is the app value. So, this is not that important.

[00:25:08]Then I would in an in an actual review as I uh already mentioned I go through all of those findings in detail like one by one and I look at the examples and if they are true false positives I probably fix my analyzer first and then re regenerate the findings or if it's if it's a true hit from the analyzer view I would go in and explain in all of this.

[00:25:39]So, if you if you're curious about how this report looks like, I have an example on my website as well. Maybe I can show this real quick.

[00:25:52]This is an example report.

[00:25:57]So, this one is a shorter one with 30 pages only. Uh and I can I will explain everything in detail. All the findings are in here and the findings themselves can have accepted risks like this or like an additional explanation. So in the end this is about uh documenting all of this and then if there is a violation for something that you would flag you can either put in a code comment as well or you can have the separate documentation saying no this is fine. We know that this this came up in the review but we we there is explanation on why this is is okay. Now other things like this um there are a few things throwing on on the same thing here. So we have a an ISO 10 CWE 1052 reporting.

[00:26:54]Let's go back 1052 gone 1052 excessive hard-coding literal and then also absorb 3 which is particular about error messages having hard-coded values because that is something that uh throws up in telemetry. Basically, then you don't get the error message in telemetry if you don't reuse a label with replaceable things, replaceable uh parameters.

[00:27:36]So, all of those kind of things. Let's see if we have some more missing tool tips for user documentation purposes, of course.

[00:27:53]Oh, this is No, it's not filtered. Okay.

[00:27:57]Magic numbers basically flex anything that is that's is that the one we had just had? Yes. Um it flex everything that is hardcoded basically. And I have um it's either either has to be a label or if it has to be an integer or something. I've uh like said that if we have an a procedure returning exactly one thing only.

[00:28:30]For example, here I would can I refactor a single value?

[00:28:36]No, I cannot.

[00:28:38]The way I would say that this is okay would be a procedure to say we have 24 hours in milliseconds and it's not a duration or an but in this case an integer.

[00:29:03]Not sure if that equals to the same amount, but it still flags because those are new magic numbers inside here. I would do this and then call it here.

[00:29:25]Now, this is probably a different problem. Yeah, this is a false positive.

[00:29:30]Okay.

[00:29:36]Uh, let's see.

[00:29:49]Like this and then it's not a a magic lit anymore because this is self-documenting like what this number is about.

[00:29:57]And the reason for this is I mean while a code comment here serves the same purpose. Either I need I can reuse this function to get the value and reduce duplication in code or I can uh remove that comment. Now yeah there is a parenthesis missing. Um, and then it makes it easier for me to read this because I don't need to wonder about what this number is. Um, in worst case, we wouldn't even have a code comment to explain what this equates to.

[00:30:43]Um, and then it's absolutely unreadable.

[00:30:46]So, this is why this rule exists. Also, for those kind of things, I would just use locked labels uh to I mean it's hard for me to explain why actually and I don't really know things like this I think again would be not sure if it helps readability in in that case um I think two reasons for why I would expect this to be an a label either if it really improves readability or reusability.

[00:31:32]But in this case, I'm not sure if anything of this would uh would be worth the refactor.

[00:31:40]And that means I either like either would still flag it or if it's really doesn't make any sense and just makes the code less readable then um I would probably also flag this as yeah I mean technically it is a warning but in practice we wouldn't change this because it gets worse if we do. I hope that explanation makes any sense. And let's see what see if we have something more.

[00:32:20]This is also interesting. I didn't have this before in practice. Mixed and or operations without explicit parentheses.

[00:32:30]Let's see. 783 it's part of maintainability and part of security. Let's see the explanation. So AL operators precedence rule differ from C family languages.

[00:32:53]Complex boolean expressions without parenthesis frequently evaluate incorrectly.

[00:33:12]I don't think so. In a in a pretty clearly defined what happens actually no this is this is interesting.

[00:33:26]So we have this or this or this and this.

[00:33:33]Now is it this together like or or or and then and or is it or or and the and is evaluated first. Does anybody know this? Because I don't.

[00:33:54]Let's see if we can find out in documentation.

[00:34:06]Um is it control statements, conditional statements?

[00:34:25]programming conventions.

[00:34:30]Let's see if we can find Okay, this doesn't seem to explain it.

[00:35:15]There's not either. So, I'm not sure if there is documentation about what what is evaluated. I would assume the end gets evaluated first, but not sure. So, uh I think the warning is justified that at least if it's like I mean if if it's clearly documented somewhere and I just wasn't able to find it might be the case then I think security or reliability is not an a concern but readability is a concern because like weird cases like this I think putting an extra set of parentheses or like refactoring this into procedures to uh to clearly indicate what happens.

[00:36:05]Um improves readability directly. And that way we can um yeah save discussions like we just have right now where readers of the code, developers who need to read the code afterwards need to figure out what is the condition that triggers this block.

[00:36:31]Let's see.

[00:36:33]error called without error info.

[00:36:42]I I was wondering my shed is in German.

[00:36:47]It Okay, that's interesting. The live chat view I have does automatically translate into German for me. I was already wondering why why people write German.

[00:37:01]Okay. So if I hover it, I get the English original evaluation happens left to left to right in if statements.

[00:37:09]Let's go back. Uh that means so that means we do have we do get or or or and then at the end it gets chained with an end. Is that what you mean?

[00:37:26]Which would probably make sense the way this is written. But anyways, it's I think it could be clearer.

[00:37:36]So, I was talking about um error info. Where was it?

[00:37:45]Uh there we go. This is PAT 03.

[00:37:55]errors raised without the rich error info pattern foreclose collectible errors and user actionable fix it actions in the platform supports which is basically just user experience mostly both things um so instead of just throwing errors like this and I think this is even a a good example no this is actually not a good example um because nothing you can do in inside the client to fix this error message.

[00:38:27]Let's see if we have more errors.

[00:38:53]License activation succeeded but subscription metadata is incorrect.

[00:38:57]Configuration issue.

[00:39:05]Activation failure.

[00:39:11]No subscription found error.

[00:39:15]This is maybe a good example. Instead of just saying, well, there is no license in the license list, we could create an error info object, an error info error message. Then we can offer a navigate action to open up the license page list for the users.

[00:39:37]So they can directly click on the error message onto the um onto the error message uh link. Let's see if we can open up an example. Business central action errors like this. Not sure if this is big enough. open image.

[00:40:10]There we go.

[00:40:13]This is how how it looks like. And this only works with error info. That's why I flag in reviews that using error infos is the better approach. You can also lock additional information like additional um dimensions or record IDs and things just to make it easier to also then act. I mean, if it's if it's an error the user just needs to fix themselves, doesn't really help that much to have metadata info in the background. But if it's an error that support has to fix, having additional information as custom dimensions in the uh emit is uh probably quite helpful.

[00:40:55]All of that can be done with error infos.

[00:40:59]Now this one might be also interesting.

[00:41:02]A return value is discarded.

[00:41:08]Sorry, my nose is tickling.

[00:41:13]Um 252 unchecked return value.

[00:41:28]Now you can you can open the website you can read through all of this yourself if you're interested but essentially um there are the the main idea is the function returns something and we do not check the return value. Now for things like code unit run um this might be intentional and in this case again it's an accepted thing. We we we get the warning we do say well this is not uh a problem here because it's intentional and we go on but in many cases we can catch the return value to at least throw a proper error message.

[00:42:14]Again, user experience. Um, for example, if we have an unprotected get somewhere, not sure if we do have those are evaluates.

[00:42:33]I'll come back to this example.

[00:42:39]Not sure. But here, uh, Jeremy did a very good job in not doing unprotected gets because I think he's also advocating this rule.

[00:42:53]But in general, the idea is whether it be a get or an evaluate like this. Um, there might be default platform error messages, but they don't give any reason, right? So if you have an get on a record and the get fails, it will just say, well, the record wasn't found, but the user has no idea why we need the record.

[00:43:20]He might see the filters, but beyond that, no information of of reasoning of how to fix this, why it's a problem, is it is it a system problem or a user solvable problem, like what's going on?

[00:43:33]So we can improve uh the user experience drastically by catching the return values and then throwing a proper error message or proper information or handling it somehow autofixing it whatever. There is lots of potential for improval uh improvement based on on this rule alone. And then we had another one I wanted to get back to.

[00:44:00]Um, for example, here there is a procedure, a custom procedure that returns something, right? So not sure if this has this also has error messages in here potentially but it also has a return value.

[00:44:32]So by using the or calling the function with the return value I think in in general the assumption is if a procedure returns something it returns it for a reason and just not observing the return value might be against what the function functions intention was. That's why this is a warning in my system so that I can uh look into those cases in particular and figure out is this a problem or not.

[00:45:05]In general, I would say if the return value is not necessary, the procedure shouldn't return the value and then everything's fine. If it's supposed to just throw error messages or do nothing like just work, then then the return value doesn't is not needed. But if it if it is there, I should do something with it. That's the general rule I think that applies.

[00:45:29]See if we have some more.

[00:45:34]Also happy to like if anybody has any questions uh shoot them into the chat. I try to read them uh out loud and then answer them of course. So uh this shouldn't be just a lecture of how I do code review.

[00:45:53]Um I think we had this one already.

[00:46:01]Oh no, this is another one. 1049. Find set is called without preceding filters.

[00:46:09]1049.

[00:46:11]Um reports and queries that join too many tables and nest too many subqueries against large trans.

[00:46:18]What happened? Sometimes I just click things on my this was like two buttons on my mouse. Um too many subqueries against large transaction tables generate disproportional database load and cause query timeouts. So it's a general to be cautious about calling fine set on complete tables.

[00:46:42]Now this again is flagged and I think extension licenses is a candidate of not many records but in general I want to see those and say well no in this example this is fine but in general setting filters does make sense. Now this is this is even an upgrade code unit. So we do actually want to run over all of those um and update everything correctly.

[00:47:12]So this is intentional.

[00:47:16]This is something I've added. Delete all without preceding is empty check.

[00:47:22]Not sure if anyone has seen that before.

[00:47:26]I also just noticed here's a warning missing because it should be delete all true, right? So running the trigger explicitly or like running the trigger should be the default. So this should be warned on.

[00:47:41]Uh but looking at the at the pat 12 but deletion operations without first checking whether the filter matches any rows can acquire locks unnecessarily.

[00:48:02]I think this is documented somewhere in the guidelines and things like sites as well. Not sure if you know this. The BC uh guide.dev I believe is the No.

[00:48:21]Oh, it's AL guidelines.

[00:48:24]Yeah. Here. So, here we have many things as well. And I believe can we search?

[00:48:44]Cannot.

[00:48:47]Oh yeah, there we can delete all.

[00:48:51]There we go.

[00:48:56]The created and described by Waldo. when you perform a delete all where there is nothing to delete will still perform a lock.

[00:49:05]So in general two things and it's even not complete in here. I think it wasn't updated in a while. We do check with is empty.

[00:49:15]And now one thing I can show you if we add the is empty check here.

[00:49:24]If not, yeah, this is wrong like this.

[00:49:44]Then we get another warning that the record is update is modified without an update lock and is empty also without hey preceding load fields is complete garbage. This is not what I wanted to show you but we read extension license without read isolation.

[00:50:06]I'm not sure actually I would need to check this if read isolation update lock is is necessary for for deleting uh isolation level update F detection doesn't work yet correctly.

[00:50:40]Okay, but you get the idea.

[00:50:44]H does it want assignment.

[00:50:57]Oh, this is what it detects. Okay.

[00:51:02]I like the procedure style better actually.

[00:51:07]Okay, now it's just the the false positive about set load fields on is empty, which is obviously not needed.

[00:51:12]But this is this is the way how it's expecting this to be done.

[00:51:18]Now I need to check on update locker versus delete all if that's necessary or not. But in general yes, exactly the the is empty could produce a table lock. That's what I what I showed here what Walder described on air guideline staff. This is why I have the rule. My only question is I I do want to have a table lock though I think because while theoretically probably impossible no why practically probably impossible theoretically there could be a data operation happening between this call and this call. So I check that there are records and then I call a delete all on that selection and what when I don't do not specify my update log read isolation for the is empty we cannot guarantee that the delete all performs on the same set of data that's the theory at least hardcoded URLs are uh identified that we shouldn't hardcode URLs. Now again, those might be accepted.

[00:52:58]Um yes. Uh the update lock should be released as as soon as possible, but that happens. Um right after, right?

[00:53:22]Again, interesting questions. I've never really thought about this.

[00:53:30]What happens to the update lock that I set on those records after I've deleted them? because there is nothing left to lock on, right? Does it still lock the the filtered set of data so to speak or does it really release the locks? And I think also the the benefit of isolation of the read isolation on the record variable is that the logs get released as soon as this variable goes out of scope which is directly after here.

[00:54:10]But you're absolutely right and this is again a nice observation. Let's let's see. I'm curious um if this is documented correctly.

[00:54:26]This is exactly what I what I intend with this kind of review. Now this opened on the another screen.

[00:54:38]Record instance isolation level.

[00:54:42]Um previously I only provided explicit isolation level via lock table method following example Haydens the isolation level on the record instance it takes the lock on the last row while subsequent reads won't trigger further locks to be taken such usage makes sense injection code, but it wasn't expected to introduce a lock table call causing subsequent reads against the same lock tableing difference between it doesn't specify that. So that might be a question for Microsoft to answer or to find out ourselves on the SQL server. So what what it covers is that I can have um like the the Read isolation is limited to the record instance while lock table was for the entire table.

[00:56:11]Now this is probably because of a flow field or something that propagates the lock locking onto the currency table as well. um different story but in essentially it's limited to the local instance and that's why I assume that the lock is only held as long as the variable is in scope but I can't conf confirm it right now.

[00:56:42]I need to go back through the to the video and find out the things that I flagged and then fix them afterwards or find out the details.

[00:56:51]Oh, here we have a Oh, no. There's JSON get which discards the return value.

[00:57:00]Also flag things like pragma warning this um uh suppression so I can see where things got disabled.

[00:57:13]to see if those were justified or not.

[00:57:20]This is also an interesting one, a case without an else, which again in practice you would probably say, well, we don't really have any alternatives here.

[00:57:36]But in terms of when we want to look at code through the ISO glasses uh ISO 5055 glasses we don't really care about application. This else this case doesn't have an else. If the environment information ever gets another value, this will run um silently in a wrong way because in in this case environment ID is will be empty which now can be assumed to always be populated and that is a potential problem in the code which is why I've also called this stream like how can we get to another level like how can we make the code Even better, and this is in my opinion by accounting for things like this to say, well, this case doesn't have an else and this is correct. If and and the else statement could just be like error message unexpected value. This wasn't supposed to be happening. We need to look into this, but preventing the code from producing a an unforeseeable outcome.

[00:58:48]Let's see more comments.

[00:58:54]Yes, exactly. The health branch always must be handled and this is standard for different development or programming languages which is exactly what we are doing here. We're taking the the ISO 5055 standard and applying it as good as we can to a code to say well we we account for those development standards and the ISO 555 thing is something that was I think it's well established in different in in other programming languages and is uh has been around for a long time and probably reviewed lots and lots of times for for whether it makes sense or not. Now the only thing here is whether my adaption onto the A language doesn't make sense or not.

[00:59:47]1088 see HTTP client calls without a time out block.

[00:59:59]Uh, I think there is a default timeout, but setting it explicitly um also probably doesn't hurt. So, this is also being flagged here. Uh, in the in the uh specific case you're looking at, you probably have an idea of how long this should really take and handle the fact when something Yeah. if it runs too long, you can handle it and you can handle it as explicitly.

[01:00:29]But I'm in general a bigger big fan of um setting things implicit value explicitly to show that you have thought about this. Same goes for insert. The default value is false. But if I just write this, nobody knows if this was intentional or just because I forgot to set the value to true ideally. So if I set this to false explicitly, I show that this was an an informed decision or at least a decision that I've made.

[01:01:08]So same goes for timeout. Even if I set the default time out explicitly, I I show that this was I'm not just using the default timeout. I want to have that that timeout.

[01:01:24]Let's see if there is more.

[01:01:26]Otherwise, I think we might wrap up soon.

[01:01:38]Well, this is a classic text that should be translatable, which is not should be uh labeled.

[01:01:58]Yes, if the platform will change the default, your your value will differ. And there are two possible ways this could well two possible outcomes.

[01:02:12]One would be the platform adjust its def uh its timeout default upwards. Basically a longer timeout by default.

[01:02:23]It doesn't affect your code at all.

[01:02:27]Um and the other one would be to adjusting it downwards making it a shorter default timeout. And then the question would be either your code performs the same way it did before because the default is not a cap then you your code actually is more resilient against those kind of changes or it's a default and a new cap then you either get a default an error message or your code just runs against the platform cap before your custom timeout triggers. So, I think it's still better to set it explicitly.

[01:03:12]I need to find a way to turn off those auto translations somehow in the chat. That's a little bit annoying.

[01:03:23]Um nice that I can convince you with my explanations, but I like to discuss questions like this. I really appreciate uh the comments because uh I mean I'm also not I mean I don't know everything and sometimes I also have my my uh my reasoning that I have with myself I get it wrong. So challenging my approaches is always welcome.

[01:04:05]Okay but I think apart from magic numbers and a few things in here I think we are almost done with like walking through the examples that this gives. Um yeah, let me know also in the comments maybe um afterwards like what do you think? Is this something is am I am I going overkill here? Is this what do you think is this justified? Does it add value?

[01:04:35]And uh if Yeah, I mean always welcome to try the free assessment with your extensions. Uh I do have a privacy policy and things in place that I do not keep the code or use it for anything. So if you want to get an idea of how your code holds up against what I've built here, let me know. You've seen there are a few things I still need to adjust.

[01:04:56]This is a a living project. Um the automatic review obviously doesn't surface what are false positives but it should give you a proper idea of like there are there a few things or are there many things as you have seen we have like 500 uh something 460 warnings in here and if I open up that quick assessment again uh this doesn't work reveal it still scores with like almost 90% which is a really good uh score I believe uh 85% on the ISO and this is mostly because we have a few warnings in here especially where where's the there are 260 warnings for ISO in total which is 210 in reliability which is mostly the missing validate triggers and everything, but I have tried to account for like repeated warnings like this in the scoring system and ISO counts for 50% of the overall quality scoring. So, as you can see, there are sometimes like 20 20 potential findings, but it's only two out of seven rules or three out of 18 rules. And this is what uh yeah is represented or I try to represent it this scoring pattern um and like explanation and things. So you can get this one for free to get an idea. Another comment I see value but I also see challenges.

[01:06:49]No, this is I okay uh not what I want to see for my project daily is exactly the the um the thing I do not have this enabled in my daily development either I mean I know about the rules I try to to um apply them but this is not meant to be an analyzer that is run all the time as I said it's it's a proprietary analyzer I won't publish this one because It's for my my my service. Um, but I wanted to show you what I do here.

[01:07:25]And um, now this would absolutely be too noisy for for anything like this. It's meant to surface everything to me while I do a manual code review and then I can decide on things like on findings whether those are again false positives, true positives or maybe positives that should that are justified and then should be documented.

[01:07:51]The idea is this review in general this this review is has two target groups. one is developers to improve on this one-time review output, right? So, uh taking this and reasoning for themselves or in the team or in the company whatever and figure out which places uh need improvement. The other target group is um not sure if I have that document still open, my example document.

[01:08:33]I do not. Let me open up that one again.

[01:08:38]I have examples on my website if you want to look through everything and also I tried to explain everything in so much detail. But this is a quality assessment report. There is also a signature at the end. So whenever I do this service and I provide this report in reality it will be much longer than this um then I do sign this and the idea is to document.

[01:09:04]So um when you not sure if we have anybody any non-developers here in the in the audience or later on maybe on demand um the report is targeted at like management level. So when you when you sell an app and maybe not a PTE but rather an app source app um or if you if you buy an app let like as a customer you you get to a partner and have an app develop for you um any developer says no the code is fine and you can trust them but maybe a different person confirming the code is fine documenting the code is fine is even better. And this way you have a document that is signed that has any explanation in there. Everything that was what was found, what's okay, the customer individual context, the explanation of why certain things have to be done this way. Although it on the first glance would contradict the rules.

[01:10:09]Um everything is in one document. It's signed. It can be archived. it's uh and that that way the the risk shifts from an unknown risk of like how I don't know what the code quality is as a developer of course I know this but if you if you think about anybody non-developer who is in higher management levels in the company wouldn't know exactly not for sure what this code is about and this is what I'm trying to solve here with this and with the documentation So, I hope that explains this a little bit.

[01:10:46]And again, I didn't want to do this as a as a selling session, but more like a uh a explanation of like uh how I assess code reviews and what things I do care about and giving you a little bit of an idea of uh how to maybe further improve your code against ISO 5055 standards.

[01:11:11]All right, I think wrapping up for now.

[01:11:14]How long do we have? Oh, over an hour already. All right. Um, let's keep this one not too long. Uh, again, let me let me know in the comments. Let me know uh what you think about this. Really interested in any feedback, any opinions, anything you might have. I hope the music wasn't too loud in this one. I turned it down uh quite a bit.

[01:11:35]And yeah, in case you haven't subscribed, always need to do the standard floss right at the end. Uh try to or if you want to get notified for uh future streams. Uh you might want to consider subscribing because I do not really announce them long before as I need to see how those fit into my schedule.

[01:11:57]All right, then. All that's left is saying have a nice day and talk to you next time and a nice weekend. I mean, it's Friday.

[01:12:06]All right. Bye.