7 Python Code Smells to AVOID at All Costs

[@ArjanCodes]

SOLID design principles help avoiding some of these code smells. Watch this video to cleanse yourself ;) : kzbin.info/www/bejne/poWlZGOXjbhqbbc.

[@timstoev5607]

good points, but most of that is refactoring related. In reality when you need to push it out, the entire code-base can and will get bugged up. Perfectly planned and executed projects exist only in the books and tutorials, mostly because there are no clients on the receiving end, or deployment requirements, or exotic randomness specific to the project. Reality is smelly, dirty and scary, and you, the dev, are up against it on your own. Against your team, which is fighting its own battles(hopefully project related- toxic environments are pretty bad). Against the system architect who doesn't give a shit for your 5K lines of code until it ends requiring interface redesign or comes up as a performance bottleneck during profiling. Against the boss who lives in a timeless fairytale of semi-random deadlines, just as a way to avoid the fiscal nightmare. Against the client who expects everything, requires nothing, but piles demands constantly. You are like Rambo- if you need to to kill 100 enemies with 5 bullets you need to make it work, like now(which would be great, if it was real- you needed to do it yesterday, which means you have to get that time travel stuff sorted out ASAP, from the cell, without the guards seeing you, and not chicken that's from a different movie cheater), you'll figure out the paperwork later. An yes you have to deal with the future you whining that the code you are writing now sucks, but consider that- if your future self likes the code you wrote now, you haven't learned anything while wring it, so probably programming is not your forte- maybe try QA instead On the actual points you make: Code duplication is bad, but messing with already tested and released code is worse. Even if it is such simple fix, you will be surprised how strange a released and tested code behaves once you start 'optimizing' it just before production. Underspecifying variables is bad, but if it passes code-review you are stuck with it as it gets too expensive to fix the mess as it tends to grow. The same is true with variable names, AND NO "REPLACE ALL" IS A BUG IN THE EDITOR. DO NOT USE IT UNLESS SUPERVISED BY A SENIOR SUPERVISED BY A SENIOR SUPERVISED BY A SENIOR... (SUPERVISED BY A SENIOR) Boolean flags are nasty but features on larger projects tend to come out of the blue, so learn to live with the dead weight. Just know that it is a dead weight and not a goto fix. Ignoring exceptions is a mixed bad. In general you should never do it. in reality however there are certain parts of the code that can throw, but are accessed in such fashion that the exception does not need to bubble(threading, communication, certain UIs). Then there are the order of operations problems and asynchronous callbacks(combined, because performance and shit) where you can get pretty crazy outputs, so exceptions are not really something you can rely on to normalize the code flow, but you still need to account for because the interpreter fires them. Adding loggers there is also dangerous(trying to trace a thread that throws and logs the exception can cause the entire machine to freeze), so sometimes you should simply let it go. The trick is to know where you can get away with it, but unfortunately that comes with experience. The best advice there is, if you are not sure, write comments. Make them long, don't be shy to explain yourself, you will find out that while trying to explain the smelly part in the comment you will actually find a way to fix it, and if not you will leave enough information to fix the WTF problem, that inevitably comes during reviews. isInstance(there was one more actually, just can't get the name off the top of my head) is required nuisance. I hate it in general, it should not exist, but because of the way python handles OOP it is a helpful hack. As the development progresses you will find yourself going for it, since redesign of the entire architecture is way too expensive. The other two are a bit more complicated: Using built in python functions is ok, as long as you are sticking to the python ecosystem. Modern projects however tend to stretch the stack, which inevitably will introduce other languages and frameworks. Those come with other people(hopefully) that can handle them. It is a nice fantasy that those will remain separated during the dev-cycle. In reality the teams(or you) will have to adapt parts of the stack that are outside of the particular tech domain to get features running. This is why you want to stick to programming fundamentals as much as possible, Yes if you want to cast a list to a dictionary you will use the zip function(or whatever its name was) and not write one yourself, but iterating an array does not need to be obfuscated just because python allows you to do it. You are not gaining that much performance, the amount of writing time is negligible, but the readability of the code is priceless. Do not forget that an experienced engineer, can start reading python in minutes. The language is not that complicated, so really give the guy(or yourself) a chance to do work on the project and not the code-base. The same is true for most of the modern languages. Please do not go exotic because the language supports some neat convention. Unless it gives you a real boost in performance(in which case adding a comment that points it out is really, like really neat, way better than the language feature itself actually), keep the things simple and straightforward. On the custom exceptions, you have a point, but honestly there is much that can go bad there, so I wouldn't recommend it for such video. Most of the people that watch it are probably lacking enough experience to make the call when and where to use custom exception. The policy for exception handling alone is a pretty deep topic, that is depends on the application, and/or the particular layer that you are in. So yeah, I do agree with the empty exception handlers, but lets leave custom exceptions. An overly-inspired junior being handed a relatively simple task, with the power of a simple language like python can do a lot of crazy shit, if s/he goes on and decides to showcase his/her knowledge of the language fundamentals, while forgetting that it is the programming fundamentals that matter, the language is just a tool that does the job. The latter is something that I would love to see promoted more often. I really need programmers than can solve programs, not translators from natural language to a set of computer languages. Possessing knowledge for the language is indeed important* but most modern languages are so similar from programmer point of view that the extra knowledge you throw in ends up bugging the project in general. *if you are doing C or C++ things are different, there knowing the language is required, but even more you need to know the machine you are building for, not to mention the libraries you are using. If you can cover all three inside out, you might handle the project relatively well, if you are disciplined enough that is

[@brane2379]

Who cares about Python code smells. Whole thing is c**p. Python is not even a computer language - no computer ever has actually executed python program. Instead, program is written as a series of one line jokes that cores laugh at while reading it on their free time at local bar, while laughing maniacally at each one and competing at trying to guess what shit comes in the next line. PC term for this is "interpreted language". On top of that, it comes with its own friggin ecosystem that whole distros are supposed to build araond and adapt to. And users are expect to wonder what little change in next Python microversion has shattered their duct-taped program, without any care about the task they are trying to do. I've yet to see ONE great program, written in Python, that is really worth using. Python was meant to e a duct-tape for in-house use, not as crucial component that one intends to sell.

[@punkisinthedetails1470]

Cleanse your :

[@dariuszspiewak5624]

@@brane2379 Some people say something because they have something to say. Others say something because they have to say something. Guess which group you're in...

[@littleconan7929]

the "try execept" thing always bothers me. I understand the idea, but sometime, when you are depending of an other complicated code, that might generate any Exception... well you do not know the kind of exceptions you are supposed to kind, nor if you forgot one exception type. So you execute this code and pray for nothing wrong to happen. And yes this might be the result of a poorly designed code, but you are not always able nor authorized to check/modify this code. Is it realy a good thing when for instance processing a file to have. except IoError (yes but an IOError could occur during the processing, not only during the file reading) Except OsError (the same) except ValueError except KeyError .... ... No, you just want to catach any error. The way you treat each of them is not different. And letting those error propagate at this stage will only create a stupid message error of the user such as "Invalid character at position 98"

[@andrew_ray]

Don't store monetary amounts in a float! Rounding errors and money don't mix. You should use a decimal type or integer (with the latter getting divided by 100 for display).

[@cerulity32k]

EXACTLY!

[@FasutonemuMyoji]

if I get really nervous aout it, I append _in_pennies to the name of a integer DB column or variable just incase someone else refuses to pay attention. 😂

[@cerulity32k]

Doesn’t a decimal, or quadruple, still have rounding errors, but way less significant?

[@andrew_ray]

@@cerulity32k A proper decimal type shouldn't have rounding errors since they arise from the conversion between decimal and binary. When you tell a float (or double or quad) to store 0.2, it stores something close to, but not exactly 0.2. Double and quad types will get you closer to 0.2, but still won't be exact because 0.2 cannot be represented in binary using finite digits. If you multiply that by a large number, or add together a bunch of numbers with similar errors, they compound and you get further and further from the right value and you end up buying 1000 widgets at 20 cents a piece for a total of $199.99. This isn't a problem for measured values because the rounding error is swallowed by limitations on precision, but money comes only in exact multiples of $0.01, so monetary values are exact. When you tell a decimal type to store 0.2, it stores exactly 0.2 (or 2 * 10 ^ -1), so when you buy 1000 widgets at 20 cents each, the total cost is correctly $200.00. Decimal types may still be subject to loss of precision (e.g. because your number has more decimal places than fit in an integer or long), but are immune to rounding errors at the cost of being more computationally expensive to use. If you also need to protect against loss of precision also, there are types like Java's BigDecimal, which allows arbitrary precision (subject to memory limitations) in exchange for being even less efficient.

[@seantilson8728]

@@andrew_ray thanks for this!

[@yeagerdd]

First time here. I gotta say: I'm in love with the way you explained why these changes had to be made. They're not just out of "because I say so" great job mate.

[@ArjanCodes]

Thank you so much, Diego, glad you enjoyed the video.

[@k283]

​@@ArjanCodes while you applied some correct refactoring here and there, you took a simple script and overengineered it by defining garbage enums and completely unnecessary (in this case) custom exceptions. This is also a smelly code, because it actually make the module harder to read. Keep it simple, unless there is a good reason to add your exceptions, or enums, or to use ABCs, etc.

[@Corporal-Clegg]

This man explains stuff so calmly and I love his accent.

[@ArjanCodes]

Thank you - I've been working on my accent a long time - and give it the right amount of "cheese".

[@Corporal-Clegg]

@@ArjanCodes are you Dutch?

[@ArjanCodes]

Yep! As Dutch as they come.

[@berndwarnders]

@@ArjanCodes Wow, working/living in the Netherlands for 10 years, I would have not guessed that. Learning Python at the moment in my 'free time' and I am amazed by the beauty and potential of it - as demonstrated by you (required to use C# at work ;( ).

[@yt-sh]

0:00 Intro 1:27 Explaining the example 3:12 Code smell #1: imprecise types 5:52 Code smell #2: duplicate code 7:31 Code smell #3: not using available built-in functions 8:53 Code smell #4: vague identifiers 10:05 Code smell #5: using isinstance to separate behavior 13:40 Code smell #6: using boolean flags to make a method do 2 different things 15:58 Code smell #7: catching and then ignoring exceptions 17:29 Code smell #8 (BONUS): not using custom exceptions 21:30 Final thoughts

[@proud22beme]

thanks for doing refactor walk-throughs of implementing concepts, it really does a nice job of reinforcing how it should be after refactor, a lot of tuts just stop at "do this and it will be better" and you going the extra mile is really nice!

[@ArjanCodes]

Thank you - glad you liked it!

[@thomasburette9129]

Did you notice that the refactoring at 5:52 "Code smell #2: duplicate code" is the exact reverse operation of the refactoring at 13:40 "Code smell #6: using boolean flags to make a method do 2 different things" ? In the first case we combine multiple functions into a single one by adding a parameter. In the second case we remove the parameter and split the function in two. In one case it's better to combine to avoid duplication and in another it's better to split to avoid complicated code. Goes to show programming is not about blindly memorizing language features and rules but there is a sense of 'taste' involved.

[@ArjanCodes]

Well spotted! Note that the underlying principle in both of these is actually the same: single responsibility. In the first case, we improve the definition of the responsibility, leading to less duplication and easier ways to extend (finding employees is now possible for any role). In the second case we split the method in two to separate the two responsibilities that were in the single method.

[@ighsight]

SMH I finally learned what enum does. This quickly turned from a code fragrance video into a level up the code video for me.

[@ArjanCodes]

Glad it was helpful!

[@CharleswoodSpudzyofficial]

Same. Mind blown

[@chri-k]

And I learned that Python has enums. Didn’t know that somehow.

[@ShanilPanara]

Honestly, the best python KZbin channel out there for non-beginners! Incredible! Thank you for everything, I'm learning so much from you

[@amir3515]

I feel like if I'm not doing everything in these videos I'm still a beginner 😭

[@thatone5350]

I was putting away a large refactoring job for days, but after getting this in my feed I got more confidence in myself seeing these incredibly well explained refactor examples. Managed to finish it in a few hours. Really enjoying your content.

[@ArjanCodes]

Thanks, glad you like the content!

[@jemand771]

i love how you sped up the parts where you type. usually these videos where people write code in real time get boring quickly because you have to wait for them to type something out. not you however, you're making this really entertaining to watch :D

[@ArjanCodes]

Sped up? No man, those parts are actually slowed down 😉.

[@zapazap]

@@ArjanCodes I thought it was simply a good editor autocompletion!

[@quantumjolt8663]

Great video as always! I think another thing to mention is maybe to think through what responsibilites each class has and/or should have, for example moving the pay() method to the Employee class as you did in the video, maybe move that responsibility back to the Company class with maybe an abstract payment handler that handles it, as it seems kind of weird for an Employee to have the responsibility to pay themselves. Nevertheless, thumbs up and great work!

[@ArjanCodes]

Absolutely and good point. Actually, in my composition vs inheritance video, I’m doing exactly that, also with an employee example: kzbin.info/www/bejne/Zp7GgWuQpbqFaZo.

[@Mateusz143]

came here to comment exactly the same 😅

[@temmayB]

16:40 The KeyboardInterrupt exception is not inherited from class Exception, but from BaseException (according to Python 3,7 docs). Such it is not caught here. But it would catch a bdb.BdbQuit, which is triggered when the Python application is to be aborted while being debugged with the Python debugger.

[@jackwii1472]

Deep lore

[@unsuspicious_youtuber]

There are countless amazing resources to learn python, but as far as going from there and bettering existing skills I don't think any other resource online has given me more practical tips (in one place, with place, easy to digest content) than this channel. Thanks Arjan!

[@ArjanCodes]

Glad to hear you’re finding the content helpful, Michael!

[@grrsa]

The algorithm brought me here. Love the instruction and examples. Great work!

[@ArjanCodes]

That’s really kind, thank you - happy that you like the video!

[@sebastiansoto3595]

I found you from reddit post and I'm glad I did, this video was superb. Amazing work!

[@namanguntiwar1012]

Same

[@ArjanCodes]

Thank you, glad you liked the video!

[@arkocal1611]

To extend on isinstance, as some have asked in the comments. One use case that I consider legit is when you are writing generic API functions. Since you cannot overload functions in Python, isinstance might be the cleanest solution. Consider numpy's sin function (I am not sure how it is implemented though). If it takes a float, it returns a float, if it takes a python list, it returns a numpy list. Especially if you are working with basic types, and you cannot make use of inheritance because you do not control the code using your library, this approach might be necessary. Another case is assert statements, or testing, when you want to make sure an object actually is something that you expect it to be. You might not need this kind of tests if your code is thoroughly type annotated though. Finally, if you are writing super generic magic that you need if you are developing a testing library, a framework etc. (which most people don't), or something that processes generic code objects (think pypy) it might be necessary.

[@khaleeji23]

This guy is a treasure, a real python expert on KZbin.

[@ArjanCodes]

Thank you so much, glad you liked the videos!

[@SzaboB33]

This video felt like a senior developer 1 on 1 reviewing my code, it was really helpful, thank you :)

[@Sciencedoneright]

The custom Exceptions bit was actually really helpful for me! Thank you Arjan!

[@ArjanCodes]

Glad to hear that!

[@JeremiKress]

I'm still new to Python programming and I just learned a ton in these 20 minutes!

[@ArjanCodes]

Happy to hear that the video was helpful!

[@liesdamnlies3372]

For #4, I’d argue even better would be hourly_rate_ and then the currency code, e.g. hourly_rate_CAD . That way it’s much more specific, the name is shorter, and it sets-up clarity when you continue development and potentially add other currencies. …should also be using the decimal module for handling money, not floats, echoing the “use builtins” bit from before. Edit: Good job with the thumbnail. I was furious already. XD

[@Vedurin]

Why not go the right way and implement currencies dynamically ? If you already think about having different currencies then they shouldn't be part of any variable naming.

[@MariusBelea]

Thanks for the nice explanations, seeing how these principles get applied onto code examples is a lot more informative and easier to remember than reading about them. Regarding the bonus smell, I'd suggest more emphasis on the usage (you did mention it in the video), when proposing custom exceptions. It's actually good to reuse built-in types as much as possible. Here, custom exceptions were needed, because of the metadata and that there's code somewhere else handling that error.

[@EW-mb1ih]

I've read the comments to see if someone wrote something about the custom exceptions part and its utility. In my opinion (and I am not an expert), the ValueError exception associated with its string description is sufficient and making a custom exception seems a bit overkill.

[@manonthedollar]

Everyone likes their own smell.

[@SenselessUsername]

And code smell is just code flavour.

[@gondoravalon7540]

George Carlin DID ask "You ever notice how your own farts ... smell okay?" - well, maybe not the kind of smell we're talking about, haha.

[@DilettanteProjects]

If you let smelly code ripen enough, it eventually becomes _Code Umami™_

[@Beanpapac15]

Wonderful video. As a person who only ever writes python code when I'm trying to bodge something together on a raspberry pi its really nice to see how it should be done.

[@marwensallem1397]

Hello Arjan, nice video as usual 🙌 I have a couple of questions : - This is the second time you create an exception class and you put the message as a parameter to the class. I am wondering, why don't we just create the message using the other parameters instead ? - Second question is more like a request. I am always wondering what is the best to organise the files in python ? is it like java where we create 1 file per class ? It would be useful if you can make a video talking about this subject specifically if there are some principles to follow in general. Thanks again ! see you next Friday ☺️

[@ArjanCodes]

Defining the message in the exception class itself is definitely a good option as well. I kind of did it automatically at the point where I’m raising the exception, as a habit. For your second question, I would indeed start with creating one file per class/concept. I’ll think about doing a video about code organization. That’s a good topic, thanks for the suggestion.

[@coltonmccormack8978]

I think it's important to know when one should or should not use list comprehensions. Just because something can be done as a nifty one-liner doesn't mean it's always the ideal solution. I see too many mid-level developers try and get fancy cramming logic into them to reduce characters at the expense of readability, which is just another code smell. If the two methods perform the same and one is significantly harder to read by the next developer, I'd argue that's worse. Your example does not fall into this category, but when you see people start chaining ternary logic operations in a comprehension it can get ugly quick. List comprehensions are usually a bit faster than implementing via a for loop though due to the time appending each item rather than initializing the full list with the comprehension results. Great video.

[@polyliker8065]

Thanks! This was way more useful than the lesson I had on codesmells when studying (they used the same book). These concrete examples and simple explanations of why it's smelly really help with digesting this 'dry' matter. And it isn't just useful with Python but with Java and other OO languages as well.

[@ArjanCodes]

Thank you, glad it was helpful!

[@bevintx5440]

Thanks for this enjoyable Python code refactoring video. Just FYI, “code smell” is much older than that refactoring book (1999]. The first time that I encountered “code smell” was in 1978 in a code comment, “This code stinks; hold your nose while you read it!” I’m sure it is even older than that.

[@Sciencedoneright]

I like how you're like Good code => nice smell Bad code => bad smell Lmao

[@notinterested8452]

He's a smelly guy...

[@Sciencedoneright]

@@notinterested8452 lol

[@cristobaljvp]

About #5, I've been using isinstance to raise TypeErrors with "wrong" type of inputs, I mean something like: if not isinstance(variable, list): raise TypeError("You must pass a list") is that not a good practice?

[@ArjanCodes]

That’s one of the few places where isinstance is useful, in particular if you’re getting the values from a place that you can’t check (such as from a JSON file that a user supplies at runtime). In the cases where you do know where the data is coming from at all times, I think it’s better to put these kinds of checks in unit tests.

[@Insanestab]

In those cases where duck typing is insufficient and you actually need check the type, I would recommend using pydantic rather that calling isinstance yourself. It'll look a lot cleaner and you won't need to write as much code.

[@SophieJMore]

I think that's good use of isinstance. But, instead of checking for a list, I'd instead recommend to check for a broader base type (unless you specifically need a list, which happens rarely). So, if you intend on using the variable in a for loop, check for Iterable; if you need to get elements by index or do slices, check for Sequence; if you need to change some values, check for MutableSequence etc. This way you're not preventing the user from passing a type that would actually work correctly with your function if it wasn't blocked by an isinstance check.

[@radadadadee]

What you wanted to use at 8:00 is a filter, not a list comprehension that does nothing but filter the list. In my opinion it's a better use of built-ins

[@zacky7862]

My favorite python instructor now. I've started watching all your videos. Your tutorial helped me improved my coding style.

[@ArjanCodes]

Thanks, happy this is helpful to you.

[@deemon710]

@6:50 What key combination did you use to replace all instances of "managers" with "employees" within the function??

[@sachinjogi1995]

Great examples. Can you please start a series on complete back-end Dev from scratch where you go through all concepts like handling loggers, exceptions, config management following clean architecture

[@ArjanCodes]

Thank you for the suggestion!

[@prudhvirajchowhan]

@@ArjanCodes please do this series

[@hansdietrich1496]

About 16:30: "except Exception" doesn't catch KeyboardInterrupt. KeyboardInterrupt on purpose is only a subclass of BaseException. Of course you're right with all the other criticism about that smell here.

[@NotTouchable]

Would you consider making a tutorial for creating a CI/CD Pipeline in Python? (Ideally with only open-source dependencies) I think it would be really interesting and empowering!

[@ArjanCodes]

Thank you for the suggestion! I'll make a note of this.

[@joemarriage3002]

Definitely make this a series! Watching the code be cleaned is wonderful

[@proud22beme]

if you dont mind me asking. could you do a video going over the itertools/functools builtin modules? itertools is a module i always forget what does what in, since its a relatively rare usecase, but when you can use it, it often saves a lot of time. having a overview video would be a huge help with it

[@ArjanCodes]

Thanks for the suggestion, I’ll put that on the list!

[@valorien1]

What an excellent video. The way you explain and demonstrate things makes this one of the best Python channels out there. Your choice of topics is also spot on. Just a sidenote: I don't think listcomps are considered "built-in functions" as you claim in this video. sum()/max()/any()/etc. are built-in functions. listcomps and genexes aren't.

[@ArjanCodes]

Thanks! And you're right, they're not built-in functions, but they're built into the syntax.

[@Gameplayer55055]

19:58 how can i deal with same names? I have code that uses tens of values: self.value=value Can it be prettified?

[@THEMithrandir09]

Well Python isn't dart :( You can mitigate this somewhat using dataclasses, or instead go for a kwargs dict and just store that, though that might make catching bad inputs harder and code less readable. Another way could be to split your class and compose it, but that's basically adding boilerplate to hide a class that's probably too big. It's the init function, sometimes it's just a long boring list.

[@Gameplayer55055]

@@THEMithrandir09 thanks for your reply. I was worrying about it, personally in some big parameters i use dataclasses

[@jammydodger9288]

@@Gameplayer55055 I don't know if it's quite what you want but you could use something along the lines of: for k in [*kwargs,]: setattr(self,k,kwargs[k])

[@Gameplayer55055]

@@jammydodger9288 thanks

[@petersilva037]

@@jammydodger9288 OK... for the very simple case... but more fun might be to use @propery and @x.setter ... which allows a lot clearer range checking and such... www.geeksforgeeks.org/getter-and-setter-in-python/

[@leninkennedy2760]

This is the best python video I've ever watched. Loved the way you explain each and every change

[@ArjanCodes]

Thank you, glad you enjoyed it!

[@dv_xl]

Nitpick: you introduced another smell I often find in OOO code, that is, objects (in this case the employee subclasses) taking on repetitive behavioral responsibility. In this case, having method on the employee class that interacts with something (in this case by printing their pay) rather than simply defining a method that returns a value. I think this was just caused by the simplicity of your example but just wanted to highlight this to others watching, just in case they fall into this trap while refactoring.

[@AndreRomano272]

Was going to point out this, in the real world there's much more going on, paying an employee will most likely interact with other parts of the system that Employee as no business depending on.

[@flam1ngicecream]

I'm not sure what you mean by this. Could you explain further?

[@V_baddict]

​@@flam1ngicecream Don't mind him. I don't get it either. Read the comment by Valmir Memeti. It makes 1000% more sense and I think that's what he's trying to say. I think.

[@egorsencha2428]

I absolutely love your videos, randomly found your channel and it is exactly what I was looking for. Exellent content.

[@DaniloSilva-pl3sq]

I'm still a beginner so pardon my incomplete conceptions. I know the titles Arjan used are much more precise, I'm just using generalizations so I can start understanding a video that I will rewatch some more times. 1: Using strings instead of custom types 2: Boilerplate code 3: Multiple lines instead of a list comprehension 4: Bad variable names 5: if and elifs isistance instead of polymorphism (method overriding) 6: Making methods do many things instead of creating more methods 7: try code except pass 8: Using built-in exceptions instead of custom ones

[@Alexander91]

The KZbin algorithm lead me here, first video I see of this channel. And... I like it! Nice, calm, and pleasantly explained. Good example and reasonable explanations. The video is very nice and informative! I know how long a learning journey can be learning all this by mistake, so these kind of videos are gold! Subbed :)

[@ArjanCodes]

Thank you Alexander, good to hear you enjoyed it, welcome onboard. Hopefully you'll find some extra value in some of my other videos ;).

[@Alexander91]

@@ArjanCodes already have found a lot of value in your other videos! And also this video added value for me just by reiterating and summarizing! Edited my comment a bit to better reflect this ;-)

[@tomkirbygreen]

Actionable, Entertaining and not alarming (get enough of that in the news). Thank you sir.

[@bonbonpony]

10:20 So what are the legitimate uses of `isinstance`? 14:14 What's "cohesion"?

[@fashionvella730]

after watching your cohesion coupling video i got the idea on my own to fix it and started able to hunting the bad points in the code and its just because of your cohesion coupling video

[@rick2591]

Thank you for the bonus tip on custom errors. This was something I always said I will get to later. If you do not do it up front, later never comes...

[@cristobaljvp]

I loved this kind of format, I would've done some of the mistakes shown here so is nice getting to see the process to get the "correct" reasoning. Thanks for the video!

[@ArjanCodes]

You’re most welcome!

[@spaceowl5957]

IMO a lot of his “correct” solutions can arguably be worse than the smelly solution in practice depending on context. In programming there are almost no clear guidelines. You should always think about what the cost vs benefit of doing it one or another way is. Don’t follow simple strict rules or blindly avoid “smelly code”.

[@priteshugrankar6815]

I barely know python yet I love your content. It's amazing. I hope one day you write a book (preferably online because you can keep updating it). I'd love to buy it.

[@RainbowSkyDancer]

Thank you. I've tried my hand at proper coding, but I've never been able to wrap my head around certain concepts, in the past. I subbed in the hopes of learning more in depth.

[@ArjanCodes]

Thanks Eric, hope the videos help you.

[@RainbowSkyDancer]

@@ArjanCodes this one did certainly

[@KyleDB150]

13:31 I have one issue with this: The company.employee[0].pay() method call now implies that employee[0] is the one DOING the paying, not getting paid. Say that employee was a manager, is he getting paid, paying someone, paying a bill? I dont think renaming it would help, cause that still implies the employee is taking the action. This seems like a new smell to me, what about you? I guess I'd keep a wrapper with the call: company.pay_employee(company.employee[0]) Which just runs that employee's pay method.

[@petemartinez3017]

My code is a lot more clean thanks to this video! When I write complex scripts I usually throw down as much code as I can as fast as I can. While my code is readable it isn't as easy for somebody other than myself to read as it should be.

[@ArjanCodes]

Thanks Pete, glad the video helped you.

[@QuintinMassey]

4:35 I get the use of Enums, but what if I was using ConfigParser to read in a string from a file, what’s the best way to take that string and make sure it’s one of those Enum values or Enum names?

[@nochan99]

Residual code smell: all functions rely on side effects (print statements) instead of actually manipulating a state. In other words; there is no separation of data and view.

[@Upendrahanda]

Hi @ArjanCodes. Loved this video. I am curious though, I've witnessed use of choices parameter in Django model fields in order to choose from limited values. Can the concept of Enum be used in those cases as well? If so, should we drop the use of choices at all?

[@nicoz6540]

Brilliant, in terms of exceptions, what do you think of the "look before you leap" vs "ask for forgiveness" philosophies?

[@codeman99-dev]

16:01 What? You can't catch a SyntaxError with `except Exception`.

[@ArjanCodes]

Well... more precisely, a NameError. Try the following thing (literally): try: something except Exception: pass

[@unusedTV]

Found your channel today through a random recommendation. Already learned something in the first codesmell: I didn't know about the Enum library. I built these kind of types by hand, assigning ints to constants. E.g. PRESIDENT = 1, SUPPORTDESK = 2, ROLES = {PRESIDENT, SUPPORTDESK} (and yea, there's already the risk of not updating the roles set when adding a new one.

[@Fernando31611]

Liked, subscribed, alerted, and commenting. This was amazing. The difference between debuggin with print("error here") and raising your own errors..

[@ArjanCodes]

Thank you so much! Glad that you enjoyed the video.

[@Aucacoyan]

Hi! This is my first video of your channel and you are great explaining and dividing a topic in steps. The video route me to thinking: is it really necessary to import other "advanced" topics in order to refactor your code? If I am the only person reading it, sure, why not? If I know how to use dataclasses, I would have no problem. But if I write code where other persons are involved, I would rather leave the code "not perfect" but easier to understand to more pythonistas. Maybe they don't know dataclasses too well, or never used in a project, or an inheritance of ABC looks weird. I still learn a lot from this video! Subscribed

[@shreedaghatpande1878]

Great job !! This a incredible reference for interns or freshers. Thanks for all the efforts.

[@SeamusHarper1234]

I like this video, because it's a good resource to improve after taking it all the "learn python in 30 minutes" videos =) Instant sub.

[@ArjanCodes]

Glad it was helpful!

[@AndreLaBranche]

Nice collection of code smells - I know them well :) One nit - when you broke out the holiday methods, you forgot to add a test for payout_a_holiday() - demonstrating the value of checks, guard rails, etc. Cheers!

[@marcosoliveira1538]

I think I am strange, because I think these videos really entertaining. Great Content!

[@alimohamedabdelrehim6582]

What is the shortcut that you used in 6:51? :) Thank you for the video as well really amazing!

[@MrOwnageNL]

Misschien wel de beste Python video's op YT..

[@ArjanCodes]

Dankjewel, Rowan!

[@DrDeuteron]

I so agree with the error handling. Builtin errors are for straight screwups breaking python. Custom errors are for things that break your abstractions.

[@alfonsov3190]

Thanks a lot for the video! There's a couple of things that called my attention because I tend to do them differently: - When abstracting the find_employee method, you then erased the find_managers, find_interns, ectetera, methods. What I'd have done is to keep them but let them use the find_employee method (which I'd use as private) in their implementations. This way (I'd think), the coupling of the role type is avoided in the main program. - When delegating the payment rules to the employee types, you erased the pay_employees method. However, I believe in this case it makes sense that the company pays their employees, so the method should stay in my opinion. What I'd have done is that the pay_employees method calls the employee.pay() method in its implementation. Although it seems that doing this does not do more decoupling in this particular case. Am I overdoing it? Please let me know. In any case, thanks again for your videos, I learn a lot from them :)

[@cyclogenisis]

Can you explain why super() was needed in your custom exception?

[@kareltavernier6642]

I develop a geometric library. It has a number of geometric object types, such as Point, Line, circular Arc, etc. 0:02 each defined by its dedicated class. I have a polymorphic distance function that takes two geometric objects as parameters, and returns the distance between them. This distance function contains a lot of isinstance calls to cater for each combination. A lot of isinstances is a code smell. However, I do not see how to avoid these isinstances. I also do not see a problem, the code is quite limpid. (Note that the calculation for each case is typically a single formula, so the function is not overly long.) Is there a better way?

[@jakubjakubec9693]

This was good. Like really really good. I am honestly suprised by the quality of this. Would definitely appreciate more videos like this. Also you earned a subscriber.

[@ArjanCodes]

Thank you Jakub, glad you liked it!

[@SiarheiAkhramenia]

Talking about responsibilities - when you move the 'pay' method to the Employee class, then you should probably also rename it to smth like 'receive_payment', because this is still the responsibility of the company to PAY, and it's still the responsibility of the employee to GET PAID. When you add the 'pay' method to the Employee class it basically means that the employee is supposed to pay to someone, which isn't the case of your example.

[@DaviSilveira]

Question, why do you use the int(), str(), float(), etc... to define variables. example, money: int = 100 or monthly_salary: float = 5000.00

[@ArjanCodes]

Those are type hints. Strictly speaking, they're not necessary in Python. I like to add them to clearly indicate in the examples what the types are. Also, if you're using dataclasses like I am in the example, types are required so that the dataclass knows what kind of attributes to construct.

[@DaviSilveira]

@@ArjanCodes that's awesome, thank you explaining that. :)

[@ravenecho2410]

Im silly bug i like filter instead of like list comprehension. I currently havent found an apply for like `map, filter, generator` types it would be really cool to have a ""reduce"" which is like hey, for everything make the previous computations concrete do this and discard. I always end up doing like a for loop, but it feels conceptually as the biggest gap in the _functional_ mental context, it causes me to break that mode of thought

[@borgehansen6680]

Love your style. Tempted to sign on to your course on API development.

[@ArjanCodes]

Thank you Borge, hope to welcome you in the course!

[@OlehBedrii]

I feel personally roasted, yet this is the code review I needed.

[@barrykruyssen]

Awesome. I agree with all 8 smells and I've been struggling with the errors which you have now enlightened for me. Thank you 🙂

[@ArjanCodes]

Thanks so much Barry, glad the content is helpful!

[@starsky51]

This may just be semantics, but I feel like the pay function should sit in the company class, rather than employee. The company is the entity performing the transaction after all. Maybe if the employee class had salary and salary_period variables, the company pay_employee function could use those to determine the payment type to perform.

[@ArjanCodes]

You're right that probably it would be better to separate payment from the Employee class. I didn't do it here to keep the example simple, but in a production system, that separation makes a lot of sense.

[@essamal-mansouri2689]

I really like how your keyboard sounds but I tried MX Brown and I absolutely hated the typing experience. Are Gaterons significantly different?

[@juancarloszazueta6462]

Hi why there´s not appear the employee Brenda in your example?

[@cgoodm]

Awesome video! Although like you mentioned, the code you replaced with the list comprehension was readable. So I'd like to think that there's also a "smell" consideration for code that's written to be intentionally readable. And while list comprehensions are cool and have less... odor? Perhaps they aren't the best choice if you're trying to place olfactory hints for non-experts to follow along with? Just a thought, and loved the video, I'm still a noob and have yet to put classes or methods to use in my code but watching you write really helped give me a higher level understanding I think. Thanks

[@bloodgain]

This "non-experts" thing comes up frequently for many languages, and it leads to bad code. You are not responsible for writing code for inexperienced developers in a language. Assume your code will be maintained by a competent developer. Make your code readable _for them._ Of course, that doesn't mean you should write code that is unusual, esoteric, or "clever", unless you've come back to optimize something, and commented what and why thoroughly. In terms of Python, you should write "Pythonic" code. List comprehensions are Pythonic.

[@ianrickey208]

The sad part of my comment is that Arjan's examples have represented my "cleaned up" code... The happy part of this comment is that I am learning some amazing design guidelines! While I am loving my Python education, there are times I really miss the simplicity of K&R C.

[@ArjanCodes]

Glad it's helpful! I really enjoy C as well (though it's been almost 2 decades since I wrote a single line of C code).

[@photaudiotech5550]

Thanks for the vid. Clean and nice. One remark: “VacationDayshortageError” why adding error which context somebody can confuse the class to something else, I mean “raise VacationDaysShortage()” is unambiguous. I think adding Error or Exception at the end of exceptions is one of a bad habit that add tiny noise to code reading.

[@ArjanCodes]

I'm trying to follow PEP8 (see: peps.python.org/pep-0008/) as much as possible for consistency. This is what PEP8 says about Exception class naming: "you should use the suffix “Error” on your exception names (if the exception actually is an error)." If this convention changes, I'm happy to change along with it.

[@victos-vertex]

Regarding #5: I once had to update a dictionary of arbitrary depth with another dictionary. This could be done using recursion with isinstance like so: def update_dictionary(dictionary:dict, update:dict): for key, value in update.items(): if isinstance(value, dict): sub_dictionary = dictionary.setdefault(key, {}) update_dictionary(sub_dictionary, value) else: dictionary[key] = value return dictionary What would your solution to the given problem look like in this case?

[@ArjanCodes]

Good one, I think this is one of the few cases where using isinstance makes sense.

[@apwatts]

Wonderful, clear and concise, only one problem.... I now have to go and refactor my project, no worries, only 10k lines or so.

[@ArjanCodes]

Haha, thanks & I know the feeling 😉

[@edgeeffect]

The intro to this is superb!!!

[@ArjanCodes]

Thank you 😊

[@tobiaskarlsson9771]

Between the two options of (1) using a string for the role and (2) having hard-coded options in the code, I believe I prefer the string. This reminds me of programmers who choose to use a fixed number of digits for phone numbers.

[@andrestricker4118]

Wouldnt a database provide the role options in real life? I believe that's where it belongs.

[@killermonkey1392]

For anything that has a fixed number of options, an enum is the way to go. Not only does this protect you against typos, but equality checks are also much faster as enum entries are usually integral types.

[@borgehansen6680]

Yes, I thought the same. Hard coding values that really belong in a database - wouldn’t that be considered a code smell?

[@Septem_150]

How would you avoid isinstance when using strongly typed python code, except by disabling mypy warnings?

[@patrick6116]

I didn’t really understand the abstract method when he explained the pay function. Why does one do that?

[@MyMattinthehat]

This is so nice to see. I’m glad I was able to keep up and do implement a lot of these already but I did get lost on the “-> None” and that bonus question is as lost on me as far as how the error class was created (some learning to do there fore sure). For reference, I’m no swe but write scripts a lot for data science and business automations (like reports, usually). Easy subscribe! Also you type like a machine!

[@D0Samp]

Every function returns a value even when not explicitly doing so, which is the nothingness value known as None (or null or nil in other programming languages). It has a type of NoneType, but for convenience you can write just "None" as a type hint. The interactive Python shell also prints literally nothing if something evaluates as None, which can be seen comparing print("Hello") with sys.stdout.write("Hello "), which besides the actual writing returns the number of characters written.

[@LabEveryday]

Your understanding of Python is top notch!

[@ArjanCodes]

Thank you so much!

[@Omar-kw5ui]

Which of these 4 books that you recommend in your description would you recommend to start with? Thank you!

[@PaulSukys]

I likely missed something, but why are `-> None` type hints added? That's the typical default and is generally redundant, no?

[@ArjanCodes]

I try to be as explicit as possible in these examples. I don't think it's redundant - the -> None type hint helps clarify that a function or method doesn't return a result. Even if this is default the case, you don't know it unless you look in the body of the method/function and see there is no (or an empty) return statement. In other typed languages such as Java or C# you also need to specify this explicitly (using void) so I think it's good that I do it here as well.

[@asdfghjkl36958]

I’m so glad I discovered your channel! Amazing stuff, thank so much

[@ArjanCodes]

Thanks - I'm happy that you like the videos!

[@Trammael]

How does `enum is enum` test work? Surely not a single ton like ‘None’?

[@technodiver9035]

Hi, Arjan, I really enjoy your channel.. maybe you'll answer a question -> I've been trying to get that color formatting for over a week. Mine formats but not like yours. Mine doesn't change color of constants/globals, when I type cast it stays white. Too much of my coding text is white where I want it color formatted. I watched your video on it but I haven't seen a difference. Some of the extensions you use I can't find. Is it because I use vscodium instead of vscode??

[@Hamsters_Rage]

are employee classes still a dataclass with pay method implemented? how real pay method are going to be implemented (like subtracting money from company account) if they are located in employee class? can you still use auto() in enums if you are going to save enum value into any kind of persistent storage?