Dynamics Corner

Episode 335: In the Dynamics Corner Chair: Understanding Clean Code and The Importance of Code Reviews

• Natalie and Stefan • Season 3 • Episode 335

Share episode

Natalie and Stefan joined Kris and Brad in discussing Code Reviews and Clean Code. The conversation explored the topic of code reviews and clean code. Natalie and Stefan addressed the purpose of code reviews, what code reviewers should look for, and the importance of following coding conventions and style guides. They also discussed the concept of clean code, emphasizing readability, maintainability, and adherence to coding conventions. 

 

Connect with Natalie on X (Twitter) https://twitter.com/KarolakNatalie

Connect with Stefan on X (Twitter) https://twitter.com/StefanMaron

Send us a text

#MSDyn365BC #BusinessCentral #BC #DynamicsCorner

Follow Kris and Brad for more content:
https://matalino.io/bio
https://bprendergast.bio.link/

Speaker 1:

Welcome everyone to another episode of Dynamics Corner, the podcast where we dive deep into all things Microsoft Dynamics. Whether you're a seasoned expert or just starting your journey into the world of Dynamics 365, this is your place to gain insights, learn new tricks and reviewing the reviews of code. I'm your co-host, chris.

Speaker 2:

And this is Brad. This episode is recorded on August 1st 2024. Chris, chris, chris. Reviewing of code. What is the reviewing of code? Do you know what the reviewing of code is? Well, today we had the opportunity to speak with two amazing guests to talk all about code reviews, clean code and several other points With us. Today we spoke with Natalie Karalik and Stephen Martin. Good afternoon.

Speaker 3:

Hello, I'm fine. How are you Doing very well? Thank you, good afternoon Hello.

Speaker 2:

I'm fine, how are you Doing? Very well, thank you.

Speaker 2:

Good morning, good morning, good afternoon Stefan and Natalie. Great to see you. I've been looking forward to talking with you both. I have a lot of things I'd like to talk with you about. You know, I hear a lot about source control management, hear a lot about test automation, hear a lot about pipeline CICD, but one thing that comes in with the source code source control management. I hear people use different terms. Maybe we can get into that a little bit as well, but one of the things is code reviews. You know, everyone talks about let's do code reviews, let's talk about pull requests, let's talk about a number of different things, and I thought who else could we talk to about code reviews and clean code than the two of you? So, before we jump into that conversation, though, would you mind telling everyone who's listening a little bit about yourselves? We can start with Natalie.

Speaker 3:

Yeah, hello everybody. So my name is Nathalie Karolak. I'm located in Germany, working for Cosmo Consult as a programmer for solutions published to the AppSource, so this is my main focus. I'm doing this since quite a long time already. I'm in the business since 2006 and I'm also a Microsoft MVP for business solutions, and I'm also a Microsoft MVP for.

Speaker 2:

Business Solutions. That's right, congratulations. Fantastic Congrats you had received that recently, and also this is your second time speaking with us. We were fortunate and lucky enough to have you speak with us early on while we're still trying to figure all this out, so we appreciate your support and speaking with us then and also returning to speak with us again. Stefan sir, how are you doing?

Speaker 4:

Yeah, thanks, I'm doing good. Hello everyone, I'm Stefan and I've been working with NAV since I started with NAV actually, so many folks as of today are not familiar with NAV anymore, so I want to point that out With NAV 2016, so not quite as long as Natalie, and I am originally from Germany. I'm now located in Poland for almost a year now and I'm working as a freelance developer, so mostly doing coding stuff, but then again all the as Brad already mentioned all the stuff that comes with us going through pipelines and code reviews and clean coding and stuff excellent, great.

Speaker 2:

And you also have the BC coding stream which I like to watch. Unfortunately, I see that you had moved it over to YouTube. I have to try to figure out the timing on that, but that's. I appreciate seeing that. It's weird. Sometimes I just put it on and play it and just watch when I'm working or do whatever. So it's nice to have stuff like that. I kind of feel like I'm in the office with you or something.

Speaker 4:

Well, that's the intention.

Speaker 2:

Yes, well, the intention worked. I'm sure many others listen to it or watch it, and if they don't, um, I suggest they do so. When it comes to code reviews, um, and I deal with it, I get asked this often, even within you know, when I'm working, because we're talking, okay, well, we're going to write code, we're going to do a pull request, now someone's going to do a code review, and what is a code review, and what should someone do during a code review? Because that's what I get asked and there's some basic stuff I can say. Well, you know, I look at some of the simple things. So, what is a code review? What should a code reviewer do? What should a code reviewer do? What should a code reviewer look for?

Speaker 3:

These are all the questions I have, so we'll hopefully get to them in the conversation. So what are your thoughts with that? Natalie, I know you work with code reviewers quite a bit, yeah, so if I should start, maybe I start with what it is not supposed to be. I simply get a formula called a pull request where I simply hit the approve button and that's it.

Speaker 3:

My task as a reviewer is to look at the code, to check what is possible, at least within a short time, at least looking at it directly in the pull request itself. I could also download the old branch and check it all in the pull request itself. I could also download the old branch and check it all in my development environment, but this is something that I, for example, do not do. However, I want to check formal stuff. I want to see that code applies to certain conventions, to style guides, to style guides in order to help us to read the code easily.

Speaker 3:

And the other task I have but it's not always possible is to also validate actually code, to look out for maybe errors or even just spelling mistakes, whatever it can be so simple things, but sometimes you're lucky and if you have a good reviewer, he or she can also point you to real logical bugs that even a test did not discover earlier. That's the ideal part, but at least you have the formal stuff, because this is the easiest thing to do and usually doesn't cost too much time. Yeah, but also it's always a question of how much you go into detail, but maybe this is something that Stefan first can talk about as well.

Speaker 4:

Stefan Hoferlund. Yeah well, code reviews it's such a huge topic we could probably talk for hours straight about this. It really depends how you yeah what kind of change you're reviewing, I think, especially with how much you go into detail. If I'm just looking at a small change, which also should receive reviews, I think, in my opinion, then you can maybe go a little bit more into details, or maybe you don't need to go into details at all because you can look through it in a few minutes and there is nothing special going on. And I probably go into detail for certain parts of code changes, of a feature change or something, when I feel the need to it. And then I sometimes also download the code onto my machine to be able to really do code navigation and stuff, to really look through the code change, yeah, and then other changes really don't need much detail.

Speaker 4:

I had another thought. What was it discovered was during code reviews that I saw a colleague, maybe not as familiar with Microsoft's code base, rewriting stuff or functionality that's already implemented in some of their helper code units. So it was really a possibility to point that out to them. So they learned code units, new code units from the base of the system app and reuse that and could get rid of some custom code. Yeah, so that's also a good thing.

Speaker 3:

Very important point indeed.

Speaker 2:

That is a big point and it goes with. You hit a couple points with the comments that you made and you touched upon maybe who should be doing the code review. Touched upon maybe who should be doing the code review because I also like to believe that code reviews can also be a learning experience or a teaching experience for someone, because I've had some individuals go through the code review experience that were new to AL development. Not necessarily to the code review purpose for, you know, validating the code, necessarily, but looking through it they were able to learn new things by looking at some code that others had created. So that's one aspect that I like about the code reviews. So they were doing it, not necessarily, like I said, for the approval process, but it was more of a training process.

Speaker 2:

So your points, stefan, about learning the Microsoft-based code units or the base library, is important because so many things have been added, or so many new methods, so many new code units have been added, that a lot of developers do recreate that unintentionally, you know, unintentional duplication of what Microsoft already has. So being able to have multiple people look at it give some good insight and also provide the feedback with it. I like your approach Natalie too of what it is not, and it's one of those daunting tasks as well, because it does take time to do it. So the size of the code review matters as well, because I've seen pull requests with hundreds of files and I've looked at it and I just scratched my head and I'm like, do I just click approve or do I have to go through all of these changes Sometimes?

Speaker 3:

you have to.

Speaker 2:

Yeah, that's one thing to keep into consideration, I think, is the size of the code review. So, with that, what is your take on who should be doing the code review? Is it a senior developer, a developer functional consultant type, or or how do you if you fit the code review in the process?

Speaker 3:

ideally everybody, but of course, this is not the way how we live it, due to organizational restrictions, maybe also permissions, whatever. It's a question of how your pull requests are actually set up in your environment. For example, we are using DevOps and we have some policies attached, branches that define that we have to create pull requests for everything. All of them have to be reviewed by at least one external let's say second developer, and this is very, very useful in my opinion, and I also really see the progress and quality since we switched from CIL to AL and this whole request approach. With code reviewing. You can learn so much on both sides really and prevent many mistakes, so glad.

Speaker 2:

Now you had mentioned CAL back in the old you know the older versions of NAV and how the objects were embedded within the application, within the object designer. So the objects were built within the application, within the object designer. So the objects were built as part of the application. So keeping track of changes and doing code reviews if anybody did code reviews back then were extremely difficult. I know I used to export the text files, just to track changes.

Speaker 2:

I had PowerShell scripts set up just to export the text files and then each day load them into a repository, so we could see the difference of what it changed, because it was very difficult in that environment to do it. So I'm happy to see that with AL we moved forward to Visual Studio Code and having separate files to build these extensions Exactly.

Speaker 3:

So I suppose none of us is missing these old times I'm not missing it for one moment at all.

Speaker 2:

So, finally, even doing since the 2016, so you had the opportunity to experience that as well too. Um, and it was similar process back all the way. If you go way back to in the vision when it first started, it was the same concept, other than the rotator, all the way up through. So so we had some challenges. So it's refreshing, and it's also depending upon when someone started working with it. It is a learning experience, or someone needs to adapt to the changes of the way things used to be done in Cal versus where they are with AL. And you mentioned one thing spelling mistakes. I'm famous for making a couple of those, by the way. With those Spelling mistakes, how do you determine it's a spelling mistake? And I did see a topic recently that someone spoke about is which language someone should code in. So that would actually, I think, would attribute to what constitutes a spelling mistake, because if someone's doing a code review and they're determining if the word's spelled correctly, what is your thoughts on that?

Speaker 4:

My take on that is really simple. I have a VS Code extension that checks it and if it yells at me, I review that and that would be the benefit of actually downloading the the branch to have uh, um yeah, helper tools locally to help with the code review and spelling mistakes, and that that extension would be one of them. And whenever it's it's blue, it's an info. Then I I look at it and I usually keep some of the warnings that it spits out in the, either in my user settings or in the workspace settings, as exceptions, because sometimes abbreviations are treated as a spelling mistake, although they are not. But it does a really good job also with camera casing and everything to pick the words up and check everything correctly.

Speaker 2:

So that really helps a lot which extension is that that you use for that? That sounds helpful.

Speaker 4:

I would need to check. From the top of my head I don't know, but yeah.

Speaker 2:

Can you repeat that please?

Speaker 3:

Sorry, the spell checker.

Speaker 2:

Oh, the spell checker. Okay, so the spell checker extension? Does it work with any language, or just english?

Speaker 4:

there are extensions like additional extensions per language.

Speaker 3:

I think english is base and then not you, you can load multiple languages and create your own libraries. Um, I've activated for american english and british english because because I have to develop some elements really in British English for a very specific application and this really works. So it has different libraries.

Speaker 2:

Well, that's one I know many people use it and I work with many that use it and I haven't. But I think I'm going to start as soon as we're done with this, just to see, I'm afraid, I think think sometimes to see what comes up with it, but we'll see you really didn't use it yet? No, no, no, no. Do you believe that all these years?

Speaker 3:

how can you survive any day like that?

Speaker 2:

I've made it. I've made it, but I have I know many use it because I end up sometimes seeing in the pull request the dictionaries where they add words, as you had mentioned to you know, as the info for the words that they want to accept with the spelling. So I know others use it, which is good. So, with the code review and the code review process you had mentioned what it's not and looking for structural changes and coded changes how do you base it? What do you base it on as far as when you look at these you had mentioned structural changes and content Is it based on just experience or are there certain standards that you follow as far as like a library or documented somewhere?

Speaker 3:

In my personal opinion, I try to stick to the Microsoft standards of what they are using in their base apps the base app. Plus and this is very important in the newer repositories like BC apps for the system application and the business foundation app, because other apps who depend on us or Microsoft. They should not switch between language styles. They should find the same style of code to attach to to read it easily. But I'm not telling that I'm applying everything blindly. There is always some criticism in it. I would not really stick to everything, but at least to everything that really makes sense and makes things easier to read. That's very important to me. What I have found helpful which is not followed, for example, by Microsoft is that properties within an object or whatever are sorted alphabetically, because only that way you can easily compare what's different between two versions of a file, between two different files actually. Yeah.

Speaker 2:

That I do use.

Speaker 3:

Some database structure that I I do use.

Speaker 2:

By the way, I use azl tools and I set up the code cleanup to go through and clean up all of the sorting because I found, like you, it's easier to do a compare of what had changed if things are sorted in a certain manner because that way they're aligned, even if you use it to sort the procedures.

Speaker 2:

I know there's some topic individuals have about how do you sort the procedures with the locals and the globals or do you group them together, sort them by name? But I have found in the code review process it's helpful to have it sorted and that's a valid reason for having them sorted, in my opinion as well to make it easier to do the review. So we talked about the use of a spell checker. Stefan, you mentioned downloading the source so you could have the spell checker. What about some of the other source code analyzers? Do you think that should be part of the process, because we know, with the source code as the app source cop, for example, and there's other tools available. Do you use those as part of the code view? Do you think they're helpful as part of the code review?

Speaker 4:

absolutely yes, and that's actually. I did a live stream recently about where I reviewed one of the pull requests from microsoft and that's exactly what I did. I just ran this through all the analyzers, also the the linter cop, to get all the different warnings and everything right right away in VS code and then I could just write comments where where I, where I thought it was useful. I don't need to find those things myself if there is like helper tools that can do the work for you. And I just wanted to pick up on Natalie's comment on the guidelines introduced by Microsoft. I really agree with the approach to follow them where it makes sense, and I just wanted to give an example where I actively decided to not follow their guidelines.

Speaker 4:

I think there is one rule for the variable naming that it should be exactly like the type you're using, for example, for table name. That rule is disabled in all my repositories, for example, because it makes sense to a certain degree, but there's just too many false positives where I wouldn't agree to this rule, so it doesn't make sense for me. And the object naming I decided a while ago already that I just cannot afford to have spaces in my object names, since we are limited to 30 characters, so it's a pascal case for me now through like everything. Same goes for field names. Um, so I know there was a this unwritten rule since forever that it should always be with spaces and everything, and then you end up abbreviating, then you add dots and then it's all just abbreviations, so no one knows what it actually has to mean if they don't look at the caption. So that doesn't make sense for me at all.

Speaker 2:

I have to chuckle because I I feel what you're saying with the naming, with the 30 characters, because even if you try to do a customer card ext, if you're doing a customer card extension right, and then you have to put in a fix on it, yeah you end up. People start I see crazy abbreviations. Wherever you get some of the longer pages you know, like the sales credit memo, they start dropping off words.

Speaker 4:

Or, you know, journal gets abbreviated a number of different ways and then you add up and then like, if you're doing app source or even for PDEs, you're recommended to use a suffix, an ethics, at least a prefix or a suffix of three characters. Now I like to add two extra characters per app. I'm doing so. It's five characters already for the ethics. I'm left with 25 characters, and that's just not enough if you have spaces and all this stuff.

Speaker 3:

So it's just not enough if you have spaces and all this stuff. So it's a best-case case for me for a few years already. Although your approach makes a lot of sense, I totally agree. I don't do it. I only try still, because it's just the rest of being an OG, I think it's just I'm just being used to that. Oh my God, yeah, business Central Life and OG. I think I was being used to that all my business central life. I try to create it in the old way as possible, and if the space doesn't fit there then I simply squeeze it into a Pascal case or just remove one space and then the next.

Speaker 4:

I think in the end it's all about consistency, and when I work for companies which have existing code and they want me to add code into their app or just help them develop and they decide to have the spaces, I of course adjust. So this is the second part. While Microsoft sets somewhat of the base of guidelines, I think if a company agrees for themselves, like we want to do certain things differently, this should also be, um, like always used, sorry, um, so like a company-wide rule. Set of of guidelines is the next part, the next layer, in my opinion.

Speaker 2:

So, yeah, when, when a company agrees to do like the old way, then of course, so that that is a good point where you can start that the rules and the guidelines and to use can be company specific, whatever fits the, the company that's managing the code or the code that's theirs. And as long as I agree with you, as long as you're consistent with what you're doing, it makes it easier for whoever's working with it or anybody who is going to come in to do the code rework with the code after.

Speaker 4:

I think one good example for this is this keyword I think, natalie, you posted the point where Microsoft agreed on or said that they are going to do it everywhere. So they are going to prefix variables System app, system app Okay. System app yeah, I mean. Base app is another story. System app okay. System app yeah, I mean. Base app is another story.

Speaker 4:

So they're they're going to prefix globals and functions that are in a code unit scope with this, although it's not mandatory by the language, um, and I know, or I read a lot of comments on twitter, that many people do not agree with this. So then again, if I would do a review at a company where it was agreed that they're not going to use it, then you should never use it that way, because I feel like, in my opinion, consistency is key. So if you want to use it, then use it and always, if you do not want to use it, then only use it in those specific cases where it's necessary to use it and otherwise not. In those specific cases where it's necessary to use it and otherwise not, but not switching around from code unit to code unit, because this ends up being annoying, in my opinion.

Speaker 2:

It does, which brings me to another. I have so many questions about this with code, code reviews and code processes, because you hit on a key point with this being added to the language as the language evolves and functionality evolves within the language and maybe even standards evolve with the language. Some code is created in earlier versions. Some code gets created all the way back from Cal and it's a functionality that needed to be migrated or moved or upgraded to work with AL or even in earlier versions of ALs. Maybe you can pick a version. What about bringing in and using some of that new functionality where there wasn't a standard before and there is a standard now or there's new addition now? Is that something where you fix it as you go? Should someone review and update the code base to use some of these new functionality for the standards?

Speaker 3:

there's the challenge with that as far as adoption of new techniques and practices with code that hasn't been around for the inception of those changes that's a delicate topic, if I understood it correctly, because there are different approaches and when it comes to such new standards as introducing this Microsoft, for example, they have decided that they want to make it the new standard and the base app, but they also decided that for pull requests that are now already created for any new lines of code they already want to edit, or at least allow it to edit now from then on, want to edit or at least allow it to edit now from then on, what they do not do is to choose one point at a time and convert everything in a separate pull request.

Speaker 3:

And this is what I would actually do with my apps, which are smaller than the base app or any other standard app, obviously. But when I want to switch to another coding standard, for me this is a topic for a separate pull request which is doing only that. And I don't want to have pull requests that have one main task to change some functionality, some funky stuff but then they also format this one, some totally unrelated code line, in that object changing to this and that I don't know. This is at least in my head. This is just one big mess and I don't see the the benefit of that. How do you think about that?

Speaker 4:

I think, um, it's, yeah, it's somewhat agree with you.

Speaker 4:

Um, I think my take on this is really as you said it has to be a separate code pull request and it's also like, if you change it through, like adjust something throughout the whole extension, this can introduce bugs, since you change everything, you touch the code.

Speaker 4:

Basically, if you change it, something can break, and so this has to be where outside of the group of developers.

Speaker 4:

In my opinion, if you're working on a pretend extension, the customer needs to know they need to test again and they probably need to test everything, so it should sit somewhere in in a testing environment for some time so they have time to review whether everything still works. If you're working for a product, then well, whoever like the testers, needs to know that they potentially need to retest everything because you really touched everything. The other approach is, if you do not want to do like a major change like this, I feel like when you're working on one code unit given that you don't have like code units with thousands and thousands of lines, if you have them separately, separately created and like smaller code units, I would probably convert them one by one as I work on them. If I'm working on one topic, then I can convert that topic because I know it will be tested anyways, because I'm doing changes to that topic so that I can adjust language and bring in those new language features as we go.

Speaker 2:

It's a mixed approach. I think it sounds situational. Now you mentioned a point. If you're uh doing it for a small app, maybe it's easier to to do it all at once to find you brought up some points of. You know, maybe as you work through and you go through it. Uh, you also, natalie, you mentioned uh something I I think for the the two of the topic of and we started talking, I guess, around it is what's in a pull request for a code review.

Speaker 2:

You had mentioned that upgrading functionality within the language, language functionality or language standards should be its own separate pull request, where feature or fixes should be their own separate pull request. What do you both think of? What should be in a pull request and what should be included with a pull request, and what I mean by that is is it one issue per pull request? Is it one major feature per pull request? And within the pull request? You know, if you use GitHub, you use DevOps. Those are the two popular tools. I know there are many other tools available that can be used. What documentation should be included with the pull request for the reviewer? Either one of you can jump on this one. I'm interested in hearing both of your opinions. You both have great opinions on all of this in points of view, so I'm eager to hear Stefan first.

Speaker 2:

Okay, we'll alternate. Stefan, you go first. This time We'll alternate.

Speaker 4:

Yeah, I mean again hours of potential talking First what should be an issue. I mean again hours of potential talking First what should be an issue. I mean if you and there is probably a line between what should be an issue or a feature request, possibly, and what should be an epic, with lots and lots of issues or features, depending on the platform you're using. So that shouldn't be too big either. And then, if you need to have a bigger issue, how many pull requests should you do? And I think this is also important if you want to control the or if you want to improve the quality of the code review itself. Because, as you said, if you have thousands of objects touched in a single pull request, you're not going to expect that anyone takes the time, sits down eight hours straight and just reviews everything you did there for the last three months.

Speaker 4:

So, not to mention the, the merging conflicts that arise with this, with those kind of huge changes. So I'm really in favor of small pull requests. I can imagine lots of pull requests for a single issue, as long as you have something that can be merged. It's really hard to come up with an example here, but if you're working on a feature and you can encapsulate a few things out of this feature, like first I'm going to introduce this change and then I need to introduce five change and then I'm I need to introduce five more changes. I can. I can separate those and merge them without breaking anything in the target branch I'm merging to.

Speaker 3:

I can totally imagining splitting it up just to make it easier to merge, to review and to test possibly um, I have problems with the 100% agile paradigm, like making hundreds of tiny pull requests one by one, then fixing some previous pull requests and this and that, and you have a long, long commit list of tiny changes or mostly bug fixes, that nobody can really understand. And why I'm mentioning that is I'm developing, as I said, apps for AppSource and they are also to be used on on-premise environments, and we have teams that create also on-premise extensions, maybe from our AppSource extensions and for them although no one has approached me yet to do that, but in my opinion the ideal way is that I provide such pull requests that any other extension developer can use as a pick list. They want a feature A and then this and that, and they don't want to pick 100 pull requests, but only a few with the functionality they need. They don't need to search for a bundle of pull requests for actually one big feature. So usually I try to really keep pull requests clean so they cover a specific feature topic or bug fix for something, however very encapsulated, and sometimes I do hit the situation that my pull request becomes too big in terms of the number of objects to look at.

Speaker 3:

However, even when I do that, I still take care of my commits inside that pull request that they are separated nicely so you can click through the commits and check them individually, like, for example, when I just did a code cleanup using the AZAL tools. Then per single conversion step I do a single commit so you can check them one by one. This is helpful for the reviewer. But overall I still follow the line or the paradigm. You have to keep them small enough because it's a hazard to look at two and two-thirds. But again.

Speaker 3:

I found it important to keep the commit history readable even to human beings, anybody else handing it that way.

Speaker 2:

Yes, I prefer, as you had mentioned, I prefer the smaller pull requests for the individual changes, and I too too do. Commits are tough because it depends on what you're working on. I do go back and if I'm doing a code review I will look through the commit history. But, depending upon the scope or the size of what the developer has with working on, some of their commits may be just a sense of saving and getting the code into the repository in the branch for that. Whatever they're working on Some of that I know we promote committing it to get it stored in there. In the event that you have a problem with the system that you're working on, or if somebody needs to take leave because of illness or from other emergencies, somebody else can go pick back right up where they have and see the changes that they've made. So some of that with me is also with the commits depends upon the duration of the modification that you're working on. But those large pull requests I think are very difficult to go through. And also a pull request per I don't know if you want to call it issue. I mean, again, it depends what you're working on. But one small topic. I'll leave the word as a topic so that somebody's reviewing is they're not trying to look at multiple things, they're looking at one specific topic at a time so that they can put. You know, I like to put my mind into that one topic and not try and say, okay, well, this goes with this, this goes with that. I can see the changes all for this one thing and see where it touches and be able to work with it. And then, as far as some of the comments, you know.

Speaker 2:

To go back to the code review as well, I know sometimes I go with you know I saw TNA's code review session at Directions, north America. I know he did it in Europe as well and you know I follow it and I it's one of the things that really got me thinking about this topic is even like putting in, like the nitpicking, because it you know how deep do you go with the comments that you're making? I mean, there's some things that are fundamentally wrong, but then there's other things. Sometimes I have to go. Eh, it's okay, but do we let it go? Do we not let it go? Maybe you may want to change it or think about doing it this way. So I'm with Stefan. We could talk about this for days, I think, and not cover all the code review processes.

Speaker 4:

I want to comment on that real quick. When I was reviewing the Microsoft code I had exactly that thought. Do I really complain about all the different typing mismatches and everything that the lindelkop warns about, because I really don't like those um? Or do I just let it go and just? I mean it? Functionality wise it doesn't change anything. But what I know is in microsoft's code base it's really hard to have them change anything once it's in there. So I decided to to comment on everything, because now there is the opportunity to fix those and I don't need to look at this for the next like 20 years, um. So yeah, because I'm like in in partner code, I think it's way easier to fix those kind of things afterwards. If you come across those kind of um, yeah, I wouldn't, wouldn't call it issues for the lack of a better word. But yeah, you can, you can fix those afterwards. But for microsoft's code it's a different story. Another reason.

Speaker 3:

A good argument to handle it that way, as you said, is um, I don't know how you handle it, but in my career, it was always very helpful for me to look at Microsoft code, to learn from it, to copy some patterns or whatever. And if we let now bad Microsoft code into the code base, this will multiply by everyone who will see it and work with it, and this is something that I cannot live with. This is something that I cannot live with, and so my pull requests, my code reviews sorry, always very nitty if such an adjective exists, and it can be very annoying in that, and I am also fearing that people get really irritated by that, and I hope nobody takes it personally, because even if you do that, the writer of this pull request, the author, still is king.

Speaker 3:

He or she can always decline anything or accept it or at least talk about it, and we never know what's on their mind, like how they want it to be. And I've made the experience that at Microsoft they are open to even those little nit things of code reviews. Most of them are really considered and sometimes you get a thank you oh, I didn't see that, or whatever. So if you have the time and if you have the nerve to really do that, just go for it and see what happens.

Speaker 4:

What I also did in the code review. I ended up actually doing those changes for them. I created a pull request onto the pull request branch because I said, well, I've downloaded the branch, I'm working on it and while I'm looking at it I can right away fix those little like here is a semicolon too much or here's like a casing mid-mesh. And I said, oh, here would I just add some spaces like empty lines, to just make it more readable. Just those kind of things I usually do right away in pull requests because I'm looking at it anyways, and then the reviewer still needs to review my review pull request and accept all the changes to their branch and then that way they know about what I changed but they don't need to do the work again, which I already did before.

Speaker 2:

That's a good point. Which?

Speaker 3:

wouldn't be necessary if Microsoft used all those tools available in their own repository.

Speaker 2:

It is true, because if you run through even their base OS code analyzers, some of their base code will not pass any of those as well. So it's good your point is valid, natalie, as well is if they cleaned up some of those inconsistencies and if we're using that as a model to go by, we could eliminate the multiplication of potential other bad habits, bad design or other issues with the code as well too. But sometimes I think we've all been in the case as well as oh, I'm just going to add this, I'll add this, I'll add this, and then you go back and look and say, ha, had I known this from the beginning, I would have redone this whole thing and done it this way. Uh, so there's sometimes I it goes back to your word I like likes your new word, nitty. Uh, you know when you're nitpicking at some of this stuff and just trying to, you know, get an understanding of what the I guess I have a new nickname nitty natalie.

Speaker 2:

I like that that right there. See, we can coin. That is nitty natalie. Well, you have the librarian, but so it's nitty natalie, the bc docs library.

Speaker 3:

Maybe I will need the other one, or maybe it's already my internal name in Microsoft, but nobody called me that way.

Speaker 2:

I'll have to make sure we Niddy, natalie. No, my thoughts to you is with that Niddy, with anybody who makes the comments, I always look at the intent. I mean, there's nothing wrong with constructive criticism because, as you both had mentioned, sometimes someone may not be aware of something, or they can. Always, you know, I know myself, I always want to learn to do, you know, do things better if possible, or even learn different ways to do things. It doesn't necessarily, you know, different doesn't always mean better, but sometimes difference there, and I think it's all in the intent of who's doing the review and the comments they make. If it's meant to be helpful or constructive, then people should be receptive to it and they make a decision on it and not think of them as being, you know, nitty-nattly how does that?

Speaker 1:

how does that work? If you're on your own, like if you're an independent developer, like, how do you code review? Do you find peers to help you review your code? How does that work?

Speaker 4:

sometimes there are in companies I work with, there are other developers who can do the review, but sometimes there's just no one, so I, uh, mostly review myself after a time you sleep on it first yeah, but I mean you always go back to your own code and wonder what you did there and why you did that.

Speaker 4:

So yeah, but again, mostly using tools to get as much like warnings and infos and errors up front to do as less mistakes as I possibly can. But yeah, you're absolutely right. I mean, if there is no one to review, there is no one to review, there is no one to review. And sometimes it would be better if someone would have reviewed my code.

Speaker 1:

Would you still go through the same process as if you're reviewing someone else's code or because you're on your own, do you just speed through it?

Speaker 4:

Well, to be honest with you, I mean mostly it's, uh, going through the code, getting it in, getting it tested, and if it works, it works. But, um, yeah, sometimes for for more crucial stuff, I really go back to it after a few days and review myself, as I would normally review others. Um, but most of the code I just end up being working on and reviewing and reading what again and over again, and then if I notice something, I obviously, um, yeah, correct it that's good.

Speaker 1:

It's a good habit to have because you know, I I know there's plenty of independent developers out there that they'll probably skip through some of that, unfortunately so just to kind of, we're talking about code views.

Speaker 2:

I do want to talk about clean code and what clean code is. In a minute I think there's something else that we wanted to discuss, but now I'm thinking of this from the process point of view. Stefan, you're talking about testing. Natalie, you mentioned it as well.

Speaker 2:

So you have within a repository, you'll have a main branch, and the main branch, I'm assuming, should be whatever is loaded in production, whatever is in someone's production environment. But there also is a point where there is a test environment. So someone will make a branch, that will make a modification for a branch and they'll make a pull request. What are your thoughts on having a branch for whatever's in Sandbox, the Sandbox environment, so that we know what's in testing or what somebody's working on a pull request into a test branch to have that code be tested and then, once that passes, have a pull request from the test branch into the main branch for the features that they'd want to bring forward? Or would you load code into the test branch from a developer branch and then, once it's completed, do a pull request to bring it into the main branch?

Speaker 3:

I still don't get what a test branch is, because I'm developing a development branch, a feature branch, so to speak, and when I'm finished and I'm creating the pull request, the pull request policies attached to Azure DevOps, they just run pipelines based on the newly created code, temporarily merged into the target branch, and this is then tested by running automated tests. So what is a test branch?

Speaker 2:

Well, I was thinking more of when you had user-based testing. Stefan mentioned having the users test some functionality and test some changes Automated testing. I'm a huge advocate for automated testing. That's a whole. We could talk days about that too. So, yes, when you create a pull request to get brought into, we'll call the main branch production. You could run those.

Speaker 2:

All those automated tests should be executed to ensure that they pass as function. But all of those automated tests or in some cases, when working for customers or if you're working for an end user, you still have to pass user acceptance testing in some cases. So at that point you will have a sandbox environment with changes that are loaded for the users to test afterwards at a point. I mean you can't have some automated tests for those, but in this type of environment you still would want to have user acceptance before moving it to production. So maybe it's good that you don't have to deal with that. Stefan, what are your thoughts on that? Do you have to do the same type of process or have you worked with that same type of process?

Speaker 4:

Yes, and it's challenging. Yes, and it's challenging. Through the conversation I tried to think about how that used to be before we were using extensions in the CL days, but I can't really remember anymore. So the challenge is really that you have one code base. So if you have one testing environment and one test branch and you have multiple features, what are you doing if one feature gets approved but three are still waiting for testing? How can you deploy? Because you cannot just merge the branch, because it contains all the feature branches that are ready for testing.

Speaker 4:

So I'm using AeroGo mostly and I think I'm not sure if it supports more than that one branching strategy. But my main branch is basically that what's currently currently the bleeding edge of development. If I do feature branch and if I do that because I don't do that usually and I will come to that in a minute then that will obviously be separate, but once it's done it will be merged into main and then, when I'm ready to deploy to production and that's what I would call a clean deployment then I would merge this main branch. If everything gets approved, I would create a release and that would then create a release branch. So my main branch actually is not in production. It's my latest release branch that allows me to do not clean deployments but one feature at a time, where I would then cherry-pick those changes, those commits, onto that latest release branch and then redeploy that as a new version. That's how I do that and since I'm mostly on my own, I do not do feature branches for smaller things I'm doing there is a word for that I'm doing that Backlog development or something. No, I don't remember. But what I do if something is not yet ready to deployment to production and I need to do that clean deployment, I will do feature flex.

Speaker 4:

There is a nice open source extension. I would need to look that up. I think we can put it in some description afterwards or something. But that will let you do like code. Like if basically you can switch off code, you can do application areas by features and the features can be enabled and disabled separately in the environment, per company or not per company, don't quote me on that please. But you, what you can do is you can push code to production and you just don't enable it. So the pages are not there, the code is not executed, table fields get created, because that's just how it is, but nothing runs and if you do that correctly it really works very good. So that's how I usually do that approach. Obviously, if I do have a bigger feature then I will put it on a different feature branch because it's too much of a hassle to put those feature conditions everywhere. But yeah, I mostly do development on the branch itself and then push everything to production once it's ready. That's good.

Speaker 2:

That's good, it's interesting. I like to talk and to see how others do it as well. I mean, there's many different options and again, I think it does come down to what works for the environment, as long as there's some standards and checks in place to make sure that some management of the source code, which is important. Clean code what is clean code? Who wants?

Speaker 4:

to go first. That depends who you ask.

Speaker 2:

Stefan, go ahead. We went to Natalie first last time, so we have to go to you this time.

Speaker 1:

I was about to say that it depends.

Speaker 4:

It totally depends and I think it's again what you agree on in a certain group of developers, and what that group is or how the scope is, is probably variable. But, um, yeah, there are guidelines introduced by microsoft, there are guidelines introduced by the language itself, um, for example, just to pick one, the the set load fields, um, which just plain improves performance if you use it or makes it worse if you do not use it. So that's a guideline, I consider. If you do not use it, it's not clean code, because there is something which, with no question, makes it faster and I think Waldo proved that a couple of times and asked Microsoft why it is that way. And I'm not loading the correct fields. It's still faster that way that even if you end up not loading the correct fields, it's still faster. So any time I see a read operation to the database where no setLoadFields is set, I question that.

Speaker 2:

That's a good strategy. I like that with the setLoadFields.

Speaker 4:

So there are those kind of guidelines by the language and then it's just what you agree on as style guides, like the nice-to-have features, where you're just saying, well, we do add this keyword in front of everything in our company, we follow Microsoft's guidance there. Then that's like anything that's not following this, not clean code in my opinion.

Speaker 2:

I got it Understood, Natalie. What is your take on clean code?

Speaker 3:

um, I agree, but I would start somewhere else. When I imagine clean code, I imagine a code base consisting of good structured code. Good structured code, easy to read, also to humans, with having procedures, with speaking names so one could derive what are they actually doing? I want to keep it as redundancy-free as possible. We don't want to multiply code, we want to reuse it. It's code that uses the Microsoft base as much as possible. Reuse as much as possible and following the same rules everywhere to be consistent in general. This is for me one of the most important rules, and on top of all that is everything that Stefan already mentioned To comply to some rules, conventions that we already know. It's about formatting that, luckily, the AL language itself already does for us more or less. It does for us more or less, but if a code base is really well readable and maintainable, I think then we've come close to the goal of having clean code.

Speaker 2:

You mentioned at the last sentence what I wanted to ask next about all of this, and that's where does code readability come into this? Again, we've seen topics recently where people have shared, where they talked about the tyranny operator. Does it make the code more readable or less readable? Is it better to have less lines of code that have a lot of calculations, for example, in it? Or, if you have a procedure that returns a Boolean, just have a result and just exit the word result, instead of exit and having a if not, then this, that case, this type of process when does code readability? What is your take on the code readability in here? Should you try to have as few aligns as possible or should you expand it out as much as possible to make it logically flow?

Speaker 3:

Depends. Think of when IA was introduced, that we finally could get rid of the superfluous begin-ends because the code cop or another code analyzer just warned about them. Like you don't need them here, put them away. So I did, for example, that, and this already decreased the code to some level. Yeah, help me out.

Speaker 2:

Stefan, what do you think with code readability or what can make it helpful?

Speaker 4:

Yeah, that also picks up a little bit on the take from Natalie before. I think readability sometimes comes at a cost or the other way around. If you want to improve something else, another aspect of your, your application, it might decrease readability. And the approach I'm thinking of is what vieko is showing on every session, I think recently his approach to testing and unit testing with interfaces and really extracting dependencies from your functions. That really makes it way easier to test functions. It also makes sure that functions only serve a single purpose and really a single purpose decreases dependency and like database dependency.

Speaker 4:

I never thought about that. It's and it's. It's awesome to that that he brought that up. But what I notice is that, um, readability really is not better with this. If I have like 10 different interfaces that are just there to make it easier to test those functions and if I have everything split out so every function really does one thing and one thing only, it does not help readability at all. And also, if you increase readability or if you really make a function more generic so it can be reused wherever it needs to be reused, it can decrease performance, for example. And the question always is how much do you want to increase readability at the cost of other questions, other topics, and how much do you? How much value does those other topics maybe have, and how much can you decrease readability? So it's still good enough, I would say so.

Speaker 2:

It's really not that easy no, it is, and a lot of this is, it's, it's. I hear the word depends and I get asked a lot of questions and sometimes I say the same thing. I'm like well, it depends, because, unfortunately, fortunately depends how you look at it, with development there isn't, or with a lot of things in life, it's not just development, there isn't a one-size-fits-all right. There's a lot of things that are situational, depending upon how it's going to be used, depending upon the environment you're in or depending upon you know, a number of factors, but I'm just trying to narrow it down to also get some, um, good ideas. Uh, myself, to see, I do this. I just like to learn and to pick up and to have the great conversations with, uh, some of the great members of the community that we have, such as, uh, yourselves. And now I'm going to stick with that nitty natalie.

Speaker 4:

I have to keep telling myself in my head nitty natalie nitty natalie, and just add one more thing, maybe, um, to how complexity and code readability conflict with other things in your code you need to worry about as well. That's maybe where code review can help again, so you can have others read your code and say, well, I don't have a clue what you're doing there. Either you need to document it or you need to just change the way you write this, this code, make it more, more readable, and I'm I'm not sure where I picked up that um, but basically a quote what, um, what makes a developer a senior developer or an experienced developer if they can develop complicated code and make it look easy, like complicated functionality in in easy code. Make it easy to understand. That's where what is really difficult, I think.

Speaker 2:

I like that. That is a good approach to have with that.

Speaker 3:

Maybe it's time for some confession.

Speaker 2:

Some what.

Speaker 3:

Some confession, because if anybody asks me, natalie, does your code base, is it clean? Do you produce only clean code? My answer would be no. You would for sure find many places that are not perfect, although I'm that nitty, but um, um it's. It can never be perfect, and sometimes I'm struggling with inner conflicts similar to those that Stefan already described, and sometimes it's a lack of time, and then you're not patient enough, whatever, and so my code base isn't perfect either.

Speaker 3:

And this is where, really, the code reviews from others who are as nitty as me are so important, because they remind me hey, why didn't you do that here? And what's what's happening here? I don't understand this. This is so good input, and if this is, yeah, practiced vividly, this improves really a lot, and and I really love that approach. So please don't ever uh assume because somebody like me is needy that that we are, uh, or think we are perfect ourselves. We are surely not.

Speaker 3:

And one thing I would like to pick up um, half an hour ago, I think you, uh, br, mentioned a word that was criticism or criticize. No matter whether I add 10 comments in a PR as a reviewer or 100, no matter whether they are nits or bigger issues. None of them is a criticism. They are just issues that I see, that I read. It's like reading a text and decide okay, there's a spelling mistake or I don't know. But this is really no criticism at all and if any coder out there feels like it is, this is something that needs to be worked on because it doesn't help. Needs to be worked on because it doesn't help and of course, I, as a reviewer, can reduce my amount of nits that I'm pushing in there, but it's all a question of communication.

Speaker 2:

No, I like that. And to go back to your confession, I agree with you because I look at some code that I have written and I sometimes scratch my head and say why do you do it that way? Um, well, why did you do it that way? Or you know you could have done it a different way, and sometimes it's a point in time, uh, sometimes it's a matter of understanding, it's a matter of, I think, for all of us and I mean my point in time it's maybe you weren't aware of something at the time, whatever it may be that you know, a guideline, process, practice, or even just the requirement that you had changed through the process, as it does happen to.

Speaker 2:

So I agree with you that everybody can benefit from a review and the comments of others of what they may see, because, um, you can sometimes become blind, uh, to yourself, and sometimes another fresh point of view can help help you, or another perspective I guess you could say it's the word I like to use can help you see things a little bit differently and understand, because of you know, sometimes we get set in our thoughts and our patterns ourselves that hearing and seeing the perspective of others is important and, yeah, I think that it's the word people use, the word constructive criticism. I think it should be feedback or something like a less harsh word, I guess. I guess you could say, because it is that sometimes just looking to help each other or to give different perspective, I call it like I had mentioned, just so that you can see it maybe from a different point of view and get understanding with that I feel like, in the end, we are all like working together on a code base, and that's really how I think about that.

Speaker 4:

It's just. I'm not criticizing yourself, I'm not criticizing the code you're writing. I'm just telling you that I would do that differently. Or here you did something that I wouldn't expect there to be. If it comes to logic, or if it's just those kind of small things like here's a typo or something, it's just. I mean review my mail, my email. Well, I mean review my mail, my email. Well, I would write it differently.

Speaker 4:

All right, take what you want. With this, I mean, you don't need to implement it, but it's just my opinion about what I, what you wanted me to review.

Speaker 2:

So no, I like that.

Speaker 1:

It's good, or even asking why you're doing it that way.

Speaker 4:

Yeah, sometimes sometimes I also in pull requests or in code reviews I ask questions like what is about the? Why did you implement it like this and not like this? I would have done it like this, why are you doing it not like this? So yeah, definitely, it's a conversation, I think.

Speaker 2:

That's good. When I used to do implementations, I used to always ask why. When I was going to requirements gatherings, I used to always ask why. Or when I was going to requirements gatherings, I used to always ask people why. And that was the toughest thing and people sometimes would get annoyed. It says, why do you do it this way? Why do you do it that way? It wasn't a why, because I thought it was wrong. It was a why?

Speaker 2:

to get a better understanding of what they're doing and why they're doing it this way, you know, can you validate it a lot. So it's, it is good. I think that we all do have the same you know intentions, which is good. Uh, which is one thing I really like. I know this. This business, central nav, the vision community, has been my life. You know, it's a lot different as a now than it was 300 years ago, but now it seems that everybody works really well together and everybody's striving to help each other out, just to lift the product up, because you know, rise and tides raise off ship and we have a bunch of passionate individuals in the community that want everything to work well and you know be, I guess you could say the best it could be, to use that loosely. So with that, stefan, natalie, thank you very much for taking the time to speak with us today. I appreciate both of your time. I appreciate the conversation, appreciate your insight.

Speaker 2:

I could literally talk for hours about this. We could go down so many different paths with this, but I've been enlightened and learned a lot from the conversation. I have a few things I want to do. I will definitely put back that spelling. I've seen so many people use it and I've never used it. And now, as you had mentioned, how could I have survived without it? I don't know. I'm going to probably put it back because, even with you know, my eyes are getting worse and worse too, so sometimes I miss that. But again, thank you for the conversation, thank you for your time. We appreciate it. If someone would like to get in contact with you to learn more about what you do uh, talk more about um code reviews, clean code or even any other rules or practices how can they get into contact with you, stefan?

Speaker 4:

um, probably best would be over twitter, or x like it's called today. Um, I think I'm accepting messages right away. Otherwise, just post a tweet and ping me and I will respond for sure.

Speaker 2:

Excellent, we'll put a link in the show notes to your ex Twitter handle. I'll keep calling it Twitter for, I think, for the rest of the time that it exists. And, natalie, how about yourself?

Speaker 3:

Very same for me. So I'm very active on my Twitter or X profile and you can also drop me a private message, I think, and if not, at least a public post, and I will easily pick it up. If you're a partner and you're on the partner network on Yammer or Viva Engage, you can also reach me there.

Speaker 2:

Perfect, thank you. Thank you both again for your time. We appreciate it, I look forward to talking with you soon. Ciao, ciao, bye. Thank you, chris, for your time for another episode of In the Dynamics Corner Chair, and thank you to our guests for participating.

Speaker 1:

Thank you, brad, for your time. It is a wonderful episode of Dynamics Corner Chair. I would also like to thank our guests for joining us. Thank you for all of our listeners tuning in as well. You can find Brad at developerlifecom, that is D-V-L-P-R-L-I-F-E dot com, and you can interact with them via Twitter D-V-L-P-R-L-I-F-E. You can also find me at matalinoio, m-a-t-a-l-i-n-o dot I O, and my Twitter handle is matalino16. And you can see those links down below in the show notes. Again, thank you everyone. Thank you and take care.

People on this episode

Head Shot

Kris

Host
Contributor

Brad

Co-host