More Python Code Smells: Avoid These 7 Smelly Snags

a couple of weeks ago i did a video about cold smells if you haven't watched that yet i'll put a link in the description i really enjoyed making that video and i think you enjoyed it too so i'm doing another one the seven smells that i'll cover today are just as bad as the smells from the previous video that i did and also give you a tip of what you can do to avoid cold smells in general let's dive in if you're new here you want to become a better software developer gain a deeper understanding of programming in general start now by subscribing and hitting the bell so you don't miss anything the example i'll be covering today is a vehicle registration system let's go through the code quickly to see what it looks like there's an enum that represents the fuel type of a vehicle we have an enum representing the status of the registration system then we have a set containing tax values for different types of fuel there's a custom exception that we're going to use later on then we have a data class vehicle model info which is a class that contains basic information about a particular vehicle model so there are things like brand model catalog price fuel type year that it was launched etc etc it has property tax that computes the tax that should be paid over this vehicle if you buy it so that depends on the fuel type and the catalog price and then there is a method that returns a string representation of the model information we have another data class called vehicle which is the actual vehicle that has a reference to the information about the model of the vehicle it has an id and it has a license plate and it also has a two-string method that returns a string representation of a vehicle then we have a class vehicle registry that maintains a list of vehicle model info instances so it knows the different models that are out there it has an online status there's a method for adding a new model to the registry that creates this vehicle model info instance for us then there are a few helper methods for generating vehicle ids and vehicle license so these use combination of functions from the random and string packages then there's a method called register vehicle that given a brand and model looks up the vehicle information generates an id and license plate and creates a vehicle instance for us the final method in the registry is online status and that reports whether the registry is currently online and finally we have a main part where we create an instance of the registry we add a couple of different vehicle models to it we verify that it's online register a new vehicle and then print out the vehicle information so let's run this code and see what it does as you can see it prints out a registry status and then it registers a vehicle with an id and the license plate so recap what is a code smell it's not a bug it's more an indication that something's wrong with your program can be a minor thing that's a really quick fix but it can also point to deeper issues in your software design today i'm gonna cover seven cold smells they're not all python specific though but they're still pretty smelly the first code smell that we're gonna look into is related to this add vehicle model info method here and the smell is that this has way too many parameters and there's two reasons for that one it's basically copying over all the parameters from vehicle model info this actually is also related to another code smell called feature envy which means that an object or method needs to know too many implementation details of another object suggesting that maybe they should be merged or split differently and the second issue is that vehicle model info here doesn't have any default values so let's solve this by doing two things the first thing we'll do is add a couple of sensible default values to vehicle model info and the second thing we'll do is change add vehicle model info to accept a vehicle model info instance instead of all these separate parameters so inside vehicle model info let's add a default value for fuel type and launch here and we're going to need the date time import here and then the second thing we'll do is simplify this function to accept a vehicle model info instance there we go that's much shorter and then the only thing we need to do here is create these vehicle model info instances and since we have default values now we can in many cases remove all these extra parameters here let's run this code and make sure that this still works and it does second cold smell is using too deep nesting let's take a look at this register vehicle method here see it's responsible for too many different things it's responsible for looking up the vehicle info information it's responsible for generating all these ids and then returning the vehicle and the second problem is that the way it's set up is not very clean because it used these nested if statements inside a for loop and the problem with deeply nested codes such as this that you have a lot of empty space here which makes it harder to read and it's better to split these things out to make it a bit more simple and cleaner so what we're going to do is separate these two responsibilities of finding vehicle info and creating the vehicle into two methods just going to copy this over and let's create a method find model info and that takes a brand and a model and that's going to return a vehicle model info instance and i'll just delete this and what we'll have this method do is that it returns an optional value so let's use the optional type here i'm going to import that there we go so when it finds the model it's going to return the vehicle info instance and otherwise we're going to return none and now we can do a register vehicle is to simply call this method to find the model so that gives us the vehicle model and if the vehicle model is valid then we're going to create the vehicle id and license and return the vehicle and otherwise we're going to return this missing error and that actually needs to be that so even though we split out register vehicle into two methods there's still a lot to improve here define model info method still has a lot of nested code and the main issue is that we have these two nested if statements that we can actually combine by using logical operation like so and that already saves us one level of indentation in this method another thing i often like to do is to handle the special cases in the beginning of a method if you look at register vehicle you see that this is actually the main thing that the method is supposed to do generate an id generate license and create a vehicle for us but there's a lot of code that's surrounding it and this part is actually nested what i generally try to do is that the main parts of a method should not be nested at all and the way to do it here is by basically turning around this condition so instead of checking whether vehicle model is valid and then doing this inside the if statement which basically introduces an extra nesting layer we can also switch it around and say if not vehicle model and then we're going to raise this vehicle info missing error there we go and now i can remove this and i can put this at a higher level of indentation and now you see that we're looking for the vehicle model we're handling the special case which is that we didn't find the model in that case we raised an error but then the main thing that the method is supposed to do is not nested at all and this helps increase the readability of your code by adhering to this and find model info you can also switch the condition around even though here it doesn't make that much of a difference because this this is actually the main thing that the method is supposed to do find the vehicle info and what you can do to swap this around is basically changing this if condition into its logical opposite and then if we didn't find it then we're going to do continue and now the return vehicle info statement is indented one less level deep in register vehicle there's one more thing you can do to make it even a bit shorter and that's by using the walrus operator warriors operator looks like this so it's column equals the difference between the regular assignment and using the walrus operator is that the walrus operator returns the result of the assignment so that means you can combine assignment and checking whether that succeeded in a single statement so what you can do is then move this over to the if statement and the only thing we need to do is add some parenthesis surrounding the operator for this to work correctly so now we've removed one more line of code from our register vehicle method personally i don't use the walrus operator that often because i find it kind of obfuscates what a method is creating in terms of resources here it's not clear anymore that this is actually creating a new vehicle model variable and if you put it at the top level then it's clear that this is what's happening the third code smell is not using the right data structure for things let's look at this vehicle models list the problem is that finding vehicle model information is kind of cumbersome because if you look at the find model info method we have to go through each of these vehicle info instances check that the brand and the model is the same and if they are then we return the vehicle and we might do this quite a lot in registry so the data structure that we chose to store the vehicle models list is perhaps not the best solution for this it might be better to have a kind of dictionary with keys so that we can find these vehicle model info objects much more easily so let's turn vehicle models here into a dictionary instead i'm going to use the dict type what we're going to use as a key because we're always looking for a combination of brand and model is a tuple of two strings the model and the brand so let's also provide that as typing information and tuple is something we need to import from the typing library there we go let's go back here again so now vehicle model is a dictionary of course we need to initialize it with the empty dictionary and now adding vehicle models of course changes slightly so that becomes this so the key is a tuple of the brands and the model and there we store the model info object so this i can remove now let's add vehicle model and finding the model is now much simpler and i see i didn't update the dock string let's also fix that right now and let's use the get function that default returns none if the vehicle can't be found and we're going to provide it tuple consisting of the brand and the model and now this we don't need anymore let's verify this still works yes it still works code smell number four let's take a look at this online status method the problem is that this uses conditional expressions which is fine that can be pretty useful but we're using a nested version of this because we're returning offline if it's not online else and then this thing here is again another conditional expression don't do this because this is really hard to read it's not really clear under what conditions which statuses are being returned so it's better if you use these conditional expressions and it becomes more complicated to split them so let's use the regular if statement to do this and then the else part we can actually return this and now this is a bit easier to understand personally i don't use these conditional if expressions that often i mainly use it when you have a choice between two things and it's really simple and that fits on one line i mean then it can be pretty clean but in all other cases i mainly use regular if statements the online status method in this example doesn't do that much i mean it doesn't check for connections and stuff like that you would probably add that if it was a more complete example but i kept it really simple here code smell number five is using wildcard imports you can see here i'm importing everything from random and from a string now generally this is a bad idea because it kind of pollutes your namespace in the module because we have all the functions that are random and all the functions that are in string and this becomes confusing relatively quickly for example here if you look where they're being used that's in generate vehicle id and generate vehicle license you see that there's a choices function there's ascii uppercase digits it's not really clear where these are coming from in general i would advise against using wildcard imports and just import the specific things that you need in this case what we can do is replace these two imports by by this there we go and now we should of course fix a few things here because it doesn't know what these things are anymore well choices comes from random this comes from string so let's update these names here now it's clear where these variables and functions are coming from there we go i think we got everything let's try this again yep that still works when you want to import a module as a rule of thumb use import module over from module import xyz unless you have a really good reason to do so a good reason could be that a module is so common like data classes or enums or date time that you can use from data classes import data class because it's also probably used all over the place in your module if a module name is too long you can shorten it import pandas spd or import numpy snp those module names are not even that long actually anyway back to coding ion code smell number six is using asymmetrical code asymmetrical code means that you're doing the same thing in different ways if you look at vehicle model info it has a method to turn it into a string vehicle also has a method to turn it into a string and they're both named differently there's no reason to do that in fact it's even better to use built-in dunder methods like str or wrapper for this so to solve this well simple let's just replace it with the str dunder method in both cases and then we also don't need the dock string here anymore and because vehicle model info now has this dumber method then actually here this also becomes way more simple because we can just simply print the info here and the rcr method is going to take care of that conversion automatically for us so then i can just remove this and i reduce this to a single line of code so we have two quite similar dunder methods that return a string str and wrapper so when do you use which one what i suggest is that you use str to produce a human readable string and wrapper to produce a string that represents the object so the string produced by wrapper you could eventually use to recreate that object in other words str is more for the users and wrapper is more for the developers now it's time for code smell number seven and after that i have a small bonus one for you number seven is using self in methods that don't need it take a look again at vehicle registry it has methods for generating the vehicle id generating vehicle license that both have self but it's actually not needed it's not used anywhere so because it's not using self it doesn't make any sense to pass it as a parameter and you should probably just turn them into static methods so let's remove self here and let's turn these into static methods one thing that you see is that this generate vehicle license method has pretty long line of code so perhaps it's a good idea to split this out to make it look a bit cleaner so let's split this out into a digit part and that's basically this and we have a letter part which is this there we go that's much better to fix this smell i used static methods python also has class methods these are not the same thing class method is a method that has access to the class instance so it can change class variables class attributes a static method can do that it simply belongs to class just because that makes sense in this case you could even argue that generating a vehicle id and generating a license shouldn't even be part of the registry but should just be put in a separate module that provides helper functions to do this now one more bonus smell and that's related to the main part here at the moment this doesn't use a main function it simply puts everything inside this if statement that's a problem because every variable that you create here is going to be available on the module level and that might lead to all kind of name clashes and shadowing and stuff like that the solution to that is to always use a separate main function so let's change this to actually use a main function there we go so it's good that you know about these smells as a software developer but if you want to avoid them in general know that there are tools to help you i use vs code as my main code editor but they also have several extensions that i use such as pylint pylance and black pylind is my linting tool of choice pylance adds a lot of extra features to vs code such as syntax highlighting automatic imports and more and finally black is a really good auto formatter a really opinionated auto format which i actually like so the combination of these three things already helps me weed out a lot of the issues in my code another thing you can do to avoid those cold smells that point to deeper design flaws is to really try and understand your problem before you start coding and i wrote a guide to help you with this it's available at ironcodes.com design guide it's totally free it contains set of bullet points my steps that i go through when i design a new software application or new feature and i think you'll find it quite helpful in structuring your thoughts so get it for free at ironcodes.com design guide that's it for today i hope you enjoyed this video thanks for watching take care and see you next time [Music] you

Channel: ArjanCodes

Views: 46,171

Keywords: code smells, python code smells, clean code, code smell, python refactoring, clean code principles, software development, code refactoring, code refactoring best practices python, code smell python, software development course, vscode python refactoring, code refactoring vscode, software development tutorial, python refactoring tips, python refactoring exercises, code smell middle man, Code smells feature envy

Id: zmWf_cHyo8s

Length: 20min 28sec (1228 seconds)

Published: Fri Jul 30 2021

Reddit Comments

[u/gstork]

really helpful video!

[u/DehiXeM]

Bonus tip is a good one :p