 Hello and welcome to Scripting for Artists. My name is Sibren and today we'll do another roast my add-on. This time it's only about one add-on called Nature Clicker. I'm changing the format a little bit and I will only be roasting one add-on at the time. This makes videos a little bit shorter and makes it a little bit less production work for me, which means that I will be able to keep going for a longer time because it doesn't eat up so much of my blender development time. On Twitter, OlaPost writes, I love the last video. Thank you very much OlaPost and as I only started coding recently I could really use a good roasting. Well that's alright then. I release this free add-on last week. It's for placing a random nature asset every time you click. Sounds useful. Let's take a look at the code. Here we are on GitHub. There is a nice description of free blender add-on for placing objects with randomization. You select the asset you want to place and every time you click during the model a random object from your selection is placed. Also add some more features. See the one minute YouTube video. So that's nice. There is a nice video about this. I will link it in the description. Basically how it works is you do a selection. You click on start clicking and then you click around and then it places a random asset on the ground plane. In a video let's take a look at the code. It's one file only so that makes things simpler. This is free add-on. That's nice but make sure that your lines are not so long that you can't read them anymore and for readability I would always recommend typing a space after a comment sign. Also in English we start our sentences with a capital letter. Also you ask for if you have any suggestions for the code please let me know. Well strictly I can't because there is no contact information in the Beal info. This is a bit weird. There's a lot of stuff being imported but especially vector is imported twice. There is a panel here which is hard to read when you zoom in like this. Then we have functions again hard to read unless you've zoomed out a lot. And then there is classes and util. If you separate one Python file into multiple sections this usually means that you actually want to have multiple Python files. I know we haven't covered this in scripting for add-ons so I can't really roast you for that. However I would suggest never to use a name called util's helpers or that kind of stuff. I mean what's the difference between a util and a function where does stuff go? Naming something util's is usually an indication that you haven't really thought about it and just plonk some things together. In this case it's just one class called settings so why not name it settings instead. After all the rest is just the regular blender stuff for registering and unregistering. And yes I know there is also to be pilot util's and no I'm not happy with that either. Let's go back to the top of the file where the panel is already fined and the naming is a bit weird. It's called clicker pt panel where I guess clicker is the category and then pt send for panel type and then the identifier is panel again which is a bit double. Looking at the other panels there's a c in front of the name here and there so I guess that stands for clicker but that means that the category all of a sudden is no longer the category and they still end in panel in the name. So I would say name them clicker pt and then whatever the role of that panel is. In this case start button because it only has to start clicking button. You could have here pt underscore ground because it has settings for the ground surface etc. One more thing that I notice here is that the layout of the code is not really consistent. Sometimes there is space equal space sometimes there is no space in front of the equal sign so I would recommend setting up your editor to use auto formatting when you save it. I will show what that looks like. Here we are in my editor visual studio code and to set up format on save you just press ctrl comma and check this checkbox format on save. Let's take a look I save the file you watch what it's doing. As you could see it moved around a little bit it had all kinds of small changes there's a space after the comment here there is no space in front of this column and also here the assignments are all done in the same way. Automating this means you don't have to think about it and then it just works and it takes your mind off of it. Clean code is always good because you definitely read code more often than you write it so taking care that your code is clean and nice to read is very very important for developers on any level. Now that we're here in my editor anyway let's take a look at an improvement that I would definitely do here. As you can see there's a lot of repetition in this code this part is repeated here as well and in the next panel and in the next panel. To put all this code into one place and then use it in all the other panels we can create what is called a mixing class. I will show you what that looks like. So I just created class called clickerPanel and that will have all the common information and this we can then use as a mixing class. So what happens here is that if blender tries to find the BL category of clickerPTPanel it won't be able to find it on the panel itself and Python will then start asking the parent in order. So it will go to clickerPanel and it will ask there if it knows BL category. In this case it does so there the search ends and then it finds clicker as the value for that attribute. We can do this on every panel now and remove those put the mixing class there on those mixing and there we are. So this has reduced quite a bit of code already and if we ever want to change that category type there's now one place where that is defined. This is another aspect of clean code there should only be one bit of code responsible for one thing. So enough about the panels let's move to the functions. There's a function addToCollectionObjectToMove which is already weird because is it added to a collection or is it moved to a collection. The function name says one thing the parameter name says a different thing. That's a bit weird. Checks if the clicker collection well capitals checks if the clicker collection exists and moves the object to this collection. So this is doing two things which is generally not a good idea for a function. Only run the code when user has checked the checkbox if bpi context seen well I have to stop you there. Never use bpi.context unless you really really have to. I'm guessing that this function is going to be called by an operator and that operator gets a context from blender. All these subsequent code should be using that context instead of using the global one. Here I'll show you an example as to why that is. Every operator can have its own context and that doesn't have to be the same as the global context. For example there's this pin here that allows me to in this object properties tab pin the tab to Suzanne the current active object. Now if I change the active object to the cube you can see that this panel still shows Suzanne. This is also the difference between context.object and context.active object. This button here is an operator and for this operator context.object will be Suzanne the pinned object whereas for an operator that is called from a menu here it doesn't have that object pinned so their context.object is actually the cube. Note that in both cases contact.active object is still the active object which is cube. This is one of the ways in which an operator can get a context that's different from the global context so just use the context that you're given and don't always go to the global one. There's more in this particular line. Comparison to true in an if statement is never necessary because the if statement already does a comparison to true so you're just effectively doubling its work. The common sense only run the code when the user has checked the checkbox and that means that if it doesn't return here there must be an else right? Oh no there is not. Return early flip your conditions return early if not yada yada yada does click collection return that means that all of this code can be an indented one step and that means you free up your brain you don't have to look at whether there is an else at the end of the function or whether it doesn't do anything when that checkbox isn't checked freeing up your brain i think is the most important part of coding stop confusing yourself the function name says one thing the parameter the name says another thing the comment here says a different thing and then there is an if that if you want to understand it you have to scroll all the way to the bottom to see that there is no else now in the next line you set collection name to the name of the collection that will be created if it doesn't exist yet i think it's a good idea to put this name into a variable but this is not the place to do that this function moves an object into the collection so i wouldn't expect this function to be the responsible bit of code that names that collection just move it to a global variable next is to try accept to get the collection if it doesn't exist yet and create it otherwise that's a big no never use a bear accept just catch whatever exception you want to catch in this case it's a key error without any exception behind the accept class you will catch each and every exception and that is not only from a technical point of view the wrong thing to do here giving the actual name of the exception that you expect here again frees up your brain when you later read it you know exactly what you were thinking at the time whichever you were expecting then on with the next bit of code this is a loop that removes the object from all the collections that it's in and then adds it to one specific collection namely the one that was just created however it's done in a very indirect weird way obj is assigned object to move so i guess your lines were getting too long there's a different way of handling that then alt collections is an empty list that is never used again and then we have a for loop that loops over object dot users collection while it's also manipulating the collections the object is in so you're in the loop you're manipulating whatever you're looking over which is also a bad idea so let's take a look at how we can improve this let's very quickly go over this code see what we can improve i start with the inside then move outward bpi data collections collection dot name is a very indirect way to get the collection collection is already the collection so to get its name and then do a look up by name to get the collection is a bit weird also it may not work the way you think it will work also now instead of having to do that accept and then bpi context scene collection see if it's maybe in there we can all remove it because we already have the collection so there is nothing to fail the second issue that we're looping over the collections and changing the collections at the same time you can just create a copy of this list so you do it like this in this notation i could do something like from two up to but not including index number five you can remove the first index to indicate from the start of the list and you can remove the last index to say up to the end of the list so this takes a copy of the entire list so now that we have a safe copy we can remove this and now that our lines are much shorter we can just use object to move again this we have this weird thing of taking the collection name and then looking up the collection that belongs to that name whereas we already have the collection here so there's a problem now that we get the collection here then we use the collection name again and then of course we no longer have the original collection because today the variables are named the same so what we can do here is for example name it clicker call to indicate that this is the clicker collection do that here as well do that here as well and then instead of this you can just say clicker call to the objects link and this is much simpler personally i would go one step further and move this and that code into two separate functions the first bit of code just gets that particular collection ensuring that it's existing this bit of code unlinks the object from all the collections that it's in so that is completely separate from the bit of code above and that means that it could be in a different function the next function is called choose object and gets self as a parameter i'm very skeptical about this because this basically means that it only gets self and it is very likely that this should be part of the class that self is pointing to so let's see where it is used yes yes this is the click operator's modal function which is huge and it calls choose object and randomization and passing self this means that it really should be part of the operator so just indent it by one level plonk it here inside the class and you can call self.choose object and self.randomization and then it's much clearer what that code belongs to apart from what i said already i think this code is fine the randomization function is a bit weird again get a random positive or negative well you already showed that you know random.choice and this is basically random.choice one or minus one but this formula and then where it's all used is it's a bit weird also i would avoid repeating bpi contact scene clicker all the time just right clicker is bpi contact scene clicker or even better the context that you pass to this function dot scene dot clicker but apart from that you take a random then turn that into a plus or minus but then here you have a random integer between 0 and 10 you divide that by 10 so you have a 0 somewhere between 0 and 1 but only in steps of one-tenth and then you multiply it with positive or negative but then also you add one so now all of a sudden is one plus minus one times something random it becomes very dodgy about what you exactly need i think what would be more appropriate is to use random.uniform and then just give a minimum and a maximum value of the random number that you want to get for example random.uniform minus one comma one will just give you a random number in that range here you're using operators for resizing and for rotating i would never do that just multiply the scale or multiply the rotation with the given values and that is much easier to read is also faster to execute the last thing that i find a bit strange is that it operates on the selected object singular if i remember your video correctly every time you click that object is randomized so this actually randomizes a very specific object so pass that object to this function and then manipulate its dot scale dot location dot rotation whatever you want to manipulate that from a coding standpoint also makes it easier because it's clear that this function operates on the object that it receives from the color let's take a quick look at that clicker operator because that is basically the decor of the add-on for one that model function is way too long i think all opposed agrees with me because there is some documentation here in the same style that you would find at the top of a function except that here it's not in a function so put it in a function split up your code because it makes everything so much easier to understand and you otherwise you get this kind of stuff here look if plane if hit if clicker align is true so when is this executed exactly putting the code into a separate function not only makes it easier to understand but also it forces you to give a name to that function it forces you to write a little bit of documentation for that function well it doesn't force you but you should anyway and as soon as you start doing something in that function that doesn't really adhere to that name that is an indication that maybe you're doing the wrong thing there maybe that should be in a separate bit of code and it really helps to streamline your thoughts as well for example this you could put into a function called align with ground plane or place on ground plane or whatever it's doing of course you could have if not plane return in there immediately and then everything else will be unindented and you get my point in the next bit of code we have the handling of the enter function which picks another random object this jumps through a bunch of hoops most importantly here to try and make sure that it actually picks a different random object so what's happening here is that the code tries to remove the any suffix like .001.002 tries to remove that from the name and then compare it to the newly selected objects to see if they differ well but what if i already selected objects that were named 3.001 etc then all of a sudden this goes wrong rather what i would do here is not use random.choice to pick a random object but random.randRange to select a random object index then if you remember the last index that you used you can just compare the indices with each other and then just keep selecting a random index until you have one that's different from the previously used index that saves you from a lot of headaches of comparing strings and manipulating strings to make the comparison work because that conceptually matches what you wanted to do then with the invoke function if context area type is few 3d do something and then all the way at the bottom there's the else just flip the condition flip your conditions move all of this and that all the way to the top and then you can un-indent the rest of the code also here be part of why are you using be part of context when you get a context here just write context.selectedObject it's the same thing but then is correct and it's less typing now look at the structure of the rest of the code we have an if we have another if we have another block that tests some things and then here again we have a block that tests some things so just flip your like just flip your conditions if context area type is not few 3d no if no if not be part of selected object return if this goes wrong return if then selected objects is one and the object is a clicker surface return that can all be put at the same level underneath each other right now what you have is a code that grows in three different directions let me show you what i mean with that let's say you have some function that takes some parameters and then it does its main thing like the thing it has to do let's call it a thing with a b and c like the way this code was made that we were just looking at if we want to have some error checking on a and b and c watch what happens with the thing so if uh a is okay for a then the thing moves over so now already we have some code above it it moved to the right and then handle error a so now what we get is we have some code above the thing we have some code below it and the main code actually indented one level now if you add another check on b then again we have to move the thing that way again we have some error handling code here and if you want to do this then again it keeps moving in three directions at the same time whereas if you flip your conditions what you get is this if not a is okay handle error a return and now i can just copy paste this i say if not b is okay handle error b and return and if if not c is okay for for c and or error c and where does the thing go now well the thing is still where it was like every error check just pushed it down but your code only grows in one direction instead of three so this is it for this episode of scripting for artists my attempt of keeping this video a bit shorter have failed but i think it was interesting nonetheless i hope that you picked up some techniques on how to improve your code so that you can free your brain from frying too much if you have any questions or comments leave them below and i will see you soon