7 Python Code Smells: Olfactory Offenses To Avoid At All Costs

do you also love the smell of nice python code in the morning it has that clinical pep 8 odor and it uses at least one list comprehension but sometimes code doesn't smell very nice it reeks of low cohesion abuse of inheritance and bad coding practices overall i'm gonna stick my nose into seven examples of really smelly code and hopefully live to tell you all about it 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 term code smell comes from martin fowler's book on refactoring i put a link to that book in the description below but what is a code smell it's not really a bug in your program but it's more of a hint that there's something wrong in your program something that you need to take a closer look at so it's possible that the program works perfectly fine but that there is an issue with the design that you need to take care of and the cold smell can help you point that out if you're a purist you consider every cold smell to be a cold stench that you need to take care of right away if you're pragmatic though you should look at cold smells on a case-by-case basis some cold smells are really easy to fix others point to a deeper design problem that is going to take some time to really fix and others are just not practical to fix at the moment let's take a look at a particularly smelly code example and make it smell all rosy again this is an example of an employee management system where employees can take holidays and we can create various types of employees so we have a employee class which is a basic representation of an employee at a company employee has a name in a row a number of vacation days and then there is a function take a holiday that either lets an employee take a holiday or pay out a number of holidays depending on this value of this boolean flag then we have a specific subclass so we have an hourly employee that's paid based on the number of worked hours and we have a salaried employee that's paid based on a fixed monthly salary a company is another class that has a list of employees we have a method for adding an employee and we have a couple of methods to find various types of employees managers vice presidents interns etc then we have a function to pay an employee that takes an employee as a parameter and then if it's a salary employee we're printing out this information if it's an hourly employee we're printing out this the main function is mainly there for testing this code and seeing that it works so we create a company we add a couple of employees we print a few find results we pay an employee and then let an employee take a holiday let's run this code to see what happens so this is what it looks like so there are no vice presidents which is true we see there is one manager there is one intern and then we're paying one employee and we're letting the employee take a holiday i'm gonna look at seven code smells in this example and then i have a little bonus for you so make sure you watch this till the end first code smell relates to the role of an employee you see we define a role here as a string and later on when we actually create these employees we're giving these employees different kinds of roles manager president enter the issue is that strings can represent basically anything we could have a role type troublemaker or slacker and we definitely don't want those kind of people at our company so string is much too broad type to specified roles it's much better to use something like an enum where you can specifically define what kind of roles there are in the company that also makes it more explicit because perhaps we don't know how to write these particular roles here we're using lowercase characters but maybe in another place in the code we might use accidentally uppercase and then we're going to have a problem because it won't work anymore enums basically solve all those problems for us because we fix what the roles are in a specific type so let's fix this code smell by adding an enum role so i'm going to import the enum type and to assign ids to the particular values inside the enum i'm going to use the auto function because i don't really care about the particular values i just want them to have a value so i'm creating a class role and that's a subclass of enum let me first add a doc string there we go and then let's define the various role types so we may have something like a president let's add a vice president and let's add a few more so now we have a couple of different useful role types then in the employee class role is no longer string but it's a row instance and let's see where we're using these roles as well so in the company we have these find methods so let's also use rules here and here as well and when we create the employees we also need to make sure that these roles are actually enum values let's run this example to make sure it still works it still works you also see that it's printing out now bit different information because it's actually no longer a string but it's an enum so that's the first code smell the second code smell is duplicate code if you look at the company class in particular these find methods they all have very similar code you have basically find all manager employees vice president employees interns and the only thing that's different is basically the the role of the employee that we're looking for generally code duplication is a bad thing because if there is a bug in this particular code then we're going to need to fix it everywhere in each of these methods it's much better to make a generic method out of this and then we have only one place where potential bugs can occur and also only one place where we need to write tests for so let's add a find employees method that's a generic version of these very specific find methods and that find employees method needs a role and let's update the docstring and then the only thing we need to change here let's also actually rename this to employees for clarity and then this is going to be the role and now i can remove each of these methods here now i can use the find employees method inside the main function to find particular employees so find vice presidents becomes finds employees role dot vice president there we go and we can do the same thing for the managers and the interns let's check that this still works it still works the third code smell is not using available built-in functions in python in principle if you look at this find employees methods i mean the code is fine the code is pretty readable it's not that long so this is fine but there are actually better ways of doing this in python for example you could reduce this entire thing to a single line of code using list comprehensions list comprehensions in general are really powerful because they allow you to basically combine generating a list mapping and filtering out things in one single line of code which is really neat let me show you how to convert this into a list comprehension that's actually really straightforward so i'm going to return a list like that and then inside the list comprehension we're going to find all the employees and we're going to take those from the employees list that's an attribute of the class but only those employees for which the role is the role that we passed as a parameter and now i can remove all this code here and we've reduced the find employees methods to this single nice short list comprehension let's run this one more time and it still works so use built-in functions because they make your life generally a lot easier code smell number four is using vague identifiers or not having units for example let's look at the hourly employee it has an hourly rate which is fine in terms of variable name but amount is a really vague identifier we don't really know what it means so we should fix this by changing that name to something a bit more meaningful for example let's turn that into hours work to make it more clear what it means there we go and now this should still work because we just did a simple refactor here so you can see that we've now changed this variable name to something that's more meaningful but we also included the units and that's actually in general much cleaner and you could arguably also say that for the hourly rate we should probably also do this so we could perhaps change this to hourly rate dollars so we know that this is actually a rate in dollars and not in i don't know euros or pounds or whatever or bitcoin that would be very expensive hourly employee 50 bitcoin well maybe in a year it won't be that much anymore gold smell 5 is using this instance whenever you see this instance anywhere in the code there should be all kinds of alarm bells going off if your code is designed well chances are you're going to use something like this instance why are we using this instance here we have a pay employee method inside a company class that gets an employee the problem is we don't know what type of employee it is it could be salary damply could be an hourly employee so we're using this instance here to determine what the type is that's of course really bad because now we introduce the direct dependency between the pay employee class and sub classes of employee it also means that whenever you add a new employee type we're going to have to extend this if else statement here and add another else to deal with that particular type of employee so it means that there is a lot of coupling here and that's that's a really bad thing is instance generally points to a more deeper issue in your design where you didn't think carefully about how the responsibilities in your code are distributed in this case paying an employee might be a function in company i could imagine that it is but the actual payment per employee type should be part of the employee class because then company can just call the pay method and you'd be done and you wouldn't have to check for its instance you would basically use the inheritance mechanism to decide for you which version of the pay method that you're going to need to call so the way to solve this code smell is not putting pay employee here but basically adding a pay methods to each employee type so you could call that instead so let's add this to salaried employee so now we have salary employee with pay method we should also add that to early employee so that's this one over here and let me copy that over there we go and this should be self obviously and now because these employees have this pay method we should probably also make that an abstract method so that we know that pay is part of every employee subtype so i'm going to the employee class and let's add this pay method as a abstract method here and let's turn employee then in an abstract class there we go let's also add a bit of typing information so that's clear what the result is actually of this method this case pay will simply print out something so the result is none and let's also add that to the specific implementations for the subclasses so there we go so now we have these pay methods i should remove this pay employee method here because we don't need it anymore so there and then instead of doing this we can do this pay this particular employee so that's also even a bit shorter than what we originally had here so let's remove that and save this and let's run this example so there we go it still does exactly the same thing but it's much cleaner in this way the sixth code smell is in this take a holiday function it has a boolean flag here which determines whether this is simply taking a single holiday or paying out five holidays the issue is that actually when you look at it this function actually does two completely different things depending on the value of this boolean flag that's a bad thing because the whole idea of methods is that they allow you to separate out responsibilities and here we're kind of combining two responsibilities into one by passing a boolean flag that lets us choose between these two the result is that take holiday as you can see here is actually pretty long function so it has low cohesion and it's also a lot harder to understand what this method is doing exactly so the fix in this case is to split out this method into two so let's say we have a take a holiday function and we have a payout holiday function so let's change this into two methods so that's the pay out a holiday and we need to copy over this part so i'll just remove that here and then put that in here fix the indentation payout holiday and we have to take a holiday method we don't need this flag anymore because it's only dealing with taking a holiday i can remove this here and let's also do it like this and then we should update the dock string as well so this is the employee that can take a single holiday and let's add a dog string as well to the pay out a holiday so there we go and now if we go down here when we call the take a holiday function we don't pass this flag anymore and now let's run this one more time it still works so you can see the results of this separation is that now we have cleaner functions so this take a holiday that has no parameters that simply lets the employee take a single holiday and we have payout holiday let's also put in the type here to make this nice and complete and while we're here let's also take a look at the last code smell before i go to the bonus which is the empty accept part here what's happening is that we put this into a try except block this piece of code and we don't want there to be any issues so what we do is if there is some kind of exception we don't expect it to happen but we're just ignoring it this is also really bad because now we're ignoring exceptions we can't do anything with particular exceptions here in this function but what we're also doing is we're basically blocking any outside code any code that uses the payout holiday method to do something with this exception and even worse actually in some cases these kind of except all blocks can ignore syntax errors or keyboard interrupt exceptions and that's really bad because that means you could basically write typo in this code and you wouldn't catch the typo because you would catch the syntax error because we're putting it in a very generic try except block if you want to catch exceptions catch exceptions of a particular type and then deal with that particular type if you can't do anything with an exception just ignore it because another part of your program might be able to do something with it so the fix here is to actually just remove this whole try except block because we're not doing anything with the exception anyway there well let's actually add an empty line here for clarity so there we go those are the seven cold smells i have one more bonus smell for you because we all like these smelly situations so let's go for one more which is related to this value error now value error is something that python raises in internal cases where there is an issue with the value that you provide to a method or anything like that it's generally not a good idea to use these built-in errors for your own custom error reporting in this case for example the error is that you don't have enough holidays left over for a payout and we print out what the remaining vacation days are the issue is that you could pass this information in text in this era but your program won't be able to do anything with it because it doesn't have the actual value if you would have the actual value you could do something like displaying the number of remaining holidays in the interface to the user but at the moment this is not really easily doable and the way to solve this is that you can actually pass this information to your own custom error types so instead of raising these generic value errors and passing along information inside strings you can create your own custom error types with their own parameters and data for example here you can create a vacation shortage error and then raise that instead of this generic value error so let's create a class that's a subclass of exception so we are patched into the exception framework and this is a custom error and we're gonna have initializer and now we can create our own custom parameters that we'd like to pass to the specific error type for example the number of days that was requested as well as the number of vacation days that are remaining and finally a message and then let's simply store this information here and let's also construct the super object which is the exception type you could probably also use a data class for this error but for now i'm i'm just leaving it like this and now let's take this type and let's scroll down to where we were raising this value error and now we can do something else for example here in payout holiday we're not raising this generic value error but let's raise a vacation days shortage error and now let's pass the parameter so we have the requested days which is simply the value of this constant and we have the remaining days which is our vacation days attribute value and then we have a message and i'll just copy over this part i forgot a comma here so now we're raising a vacation day shortage error and we're actually passing along useful information so there's an error here what's the problem let's take a look at the vacation day shortage error class oh actually i see that there's an extra underscore here yeah that solves it let's run this again still works and now let's also add this vacation a shortage error to the other method that also raises value error so that's here so here the requested days is one the remaining days is self notification is so exactly the same but let's pass this as a message instead and now we can remove this line of code here and there we basically solved all these code smells that were in the example now some of these code smells were really pretty minor things like using an enum instead of a string but some of them pointed more to a deeper problem in the design like the location of the pay method that should not be in the company but should be in the complete subtypes if you haven't done so already join my discord server to chat about software design and python stuff in general i hope you enjoyed this thanks for watching take care and see you next time

Channel: ArjanCodes

Views: 127,214

Rating: 4.9288955 out of 5

Keywords: code smells, clean code, code smell, python refactoring, code refactoring, software development, clean code tips, vscode python refactoring, code refactoring python, code quality metrics, software design, python code smells, code smells python, bad code design, bad software design, python software development, python software making, python software, python refactoring exercises, refactoring python code, code smell python, Code smells refactoring

Id: LrtnLEkOwFE

Length: 22min 9sec (1329 seconds)

Published: Fri Jun 25 2021

Reddit Comments

[u/jwink3101]

Can anyone give a TL/DW?

[u/[deleted]]

Normally I am kind of skeptic with these Python "experts," but this guy sounds like he is Dutch.

[u/BuryMeinWind]

Nice list. Definitely every mid-advanced level developer should watch and pay attention on them. Thanks for sharing!

[u/Aettos]

I've been watching your "Write better python code" series this week and just want to say thank you for all your work. The concepts you explain are perfect for my self learned intermediate-ish level. Plus, the examples you use are easy to understand and extrapolate to other uses. Plus plus, going the extra step and sharing the code in github is also a big help

[u/crawl_dht]

What vscode extension and linter are you using for liniting? It highlights syntax errors very well.

[u/zoniiic]

I'm on my way to IT transition (self taught), but right now reminding C++ from university times (doing pretty allright) and then proceeding to Python. You really bought me out with this video - so much necessary info on good coding techniques that do not only apply to Python but coding in general. Not sure if more people would appreciate it, but I would love to see more episodes on 'code smell' and good coding practices in future! Subbed!

[u/Iceberg_Bart_Simpson]

I am definitely guilty of using strings as categories and forgetting where I lowercase them to compare. It gets annoying really fast

[u/Mr_Batfleck]

Awesome content, deserves a sub. One question though, what are your thoughts on pydantic instead of dataclasses?

[u/mind_uncapped]

9:57 No bitcoin fanbois were harmed lol