More Python Code Smells: Avoid These 7 Smelly Snags
Vložit
- čas přidán 31. 05. 2024
- These are 7 code smells to avoid plus a bonus smell. I describe each smell using a Python example and then show you how to fix it. At the end of the video, I have a few general tips to help you avoid introducing code smells in the first place in your design.
For the first Python code smells video, see: • 7 Python Code Smells: ... .
The code I worked on in this episode is available here: github.com/ArjanCodes/2021-mo....
💡 Here's my FREE 7-step guide to help you consistently design great software: arjancodes.com/designguide.
🎓 Courses:
The Software Designer Mindset: www.arjancodes.com/mindset
The Software Designer Mindset Team Packages: www.arjancodes.com/sas
The Software Architect Mindset: Pre-register now! www.arjancodes.com/architect
Next Level Python: Become a Python Expert: www.arjancodes.com/next-level...
The 30-Day Design Challenge: www.arjancodes.com/30ddc
🛒 GEAR & RECOMMENDED BOOKS: kit.co/arjancodes.
💬 Join my Discord server here: discord.arjan.codes
🐦Twitter: / arjancodes
🌍LinkedIn: / arjancodes
🕵Facebook: / arjancodes
👀 Channel code reviewer board:
- Yoriz
- Ryan Laursen
- Sybren A. Stüvel
🔖 Chapters:
0:00 Intro
0:31 Explaining the example
2:45 What is a code smell?
3:04 #1 Too many parameters
5:03 #2 Too deep nesting
10:01 #3 Not using the right data structure
12:19 #4 Nested conditional expressions
13:39 #5 Using wildcard imports
15:33 #6 Asymmetrical code
16:58 #7 Using self when it's not needed
18:31 #8 (BONUS): Not using a main() function
19:02 Tips for avoiding code smells
#arjancodes #softwaredesign #python
DISCLAIMER - The links in this description might be affiliate links. If you purchase a product or service through one of those links, I may receive a small commission. There is no additional charge to you. Thanks for supporting my channel so I can continue to provide you with free content each week!
If you haven't seen the first code smells video, this is the link: czcams.com/video/LrtnLEkOwFE/video.html.
Regarding your first code smell: I feel that having meaningful, but possibly inaccurate, default values is smelly. A programmer who neglects to provide a car's fuel type by mistake should get an error, not an electric car. I would support using `None` or some enum value that represents an invalid value.
For a developer using your code base as an API, the same amount of mental effort would be needed to determine whether a parameter can be excluded because the default is the correct value as would be needed to simply include the value.
It's a balance for sure. I believe the main issue in this code smell was the feature envy part. Default values do give an idea of what a developer expects as a reasonable input but you are right that choosing good default values is hard. If you succeed though, it really makes using an API or a function much easier.
I agree and the much more problematic issue is to provide a static default for a slowly changing dimension (here: the year). If the program runs over New Year's Eve, it will be very likely being wrong and that silently while probably passing all tests and even working locally correct in your local development setting on your computer. That's a very bad thing and really very ugly to debug (in my experience, you will need multiple seniors to spot that issue on running in production). It's also a problem that might only be on one the of the servers and not on another server (maybe because there was a restart triggered by docker/Kubernetes or by the AWS service, so it is a bug not only hard to spot, but completely out of our control when it happens in production and will be even different for the same deployment on different computers).
And even worse: Very certain, all unit tests, all integration tests and all acceptance tests will pass, unless of course you are lucky and have one test exactly starting some ms before New Year's Eve and will trait it's fail very seriously, even though all consecutive tests are passing again.
A complete production maintainance nightmare.
Not sure if a linter might catch it (somehow I doubt it).
Here, the correct implementation would be either to set a factory method for the default values (IIRC dataclasses support that) or probably get rid of a default value for a year, just as there is no default value for a year. (Yes I know, the current year, but this does not have a static value, it's slowly changing).
@ArjanCodes Maybe it's worth to add a pop up ("should be a factory") that this is accidentially a huge code smell. I think, a lot of junior will follow here your example and as it is tricky to understand and to fall into this trap, this is solid potential to affect production systems doing something important.
Really good content, I would recommend this channel to anyone who wants to make their code cleaner.
Thank you James, glad you enjoy the videos.
Really appreciated the time you invested in to create this content. Videos beyond basic stuff is hard to find.
replacing 2021 with current year was quite unexpected :) a good refactoring to have a happy new year %)
In code smell 4 you don't need else because of return early pattern.
Oof ya, saw that year default and was like “oh boy, that’ll be a fun bug to find next year”
@@Glazer209 That was my thought exactly, and so when I saw your comment here I was like: "oh boy! we really are a cult, or at least I understand why 'normal' people sees us as members of a different species" :)
Thank you so much for doing these videos! They are helping me IMMENSELY on my journey to become a better developer. I have to watch and rewatch a lot of them, but over time stuff starts to stick, and it makes a huge difference.
For example, one code smell I purged from my project recently was over-using
from import thing1, thing2, thing3, thing4, thing5, ..., thing10
It added a lot of useless noise and made it tedious to swap out objects for something else from the same method.
This is a gui project with pyside6, so importing all those widgets was messy until I just used
import PySide6.QWidgets as Qw
To simply refer all things by prefixing Qw. rather than all those explicit imports.
The video you made on pydantic also helped a lot, making the way I store data much, much clearer and easier. Before, I had been using a massive, deeply nested dictionary, and remembering how to spell all those key strings was so prone to bugs. It still gives me shivers.
Thanks!
Glad to hear the videos helped you!
I believe you don’t actually need to import Tuple from typing, you can just use the buuiltin tuple, at least in more recent versions of python
You're absolutely right, I'll make sure to do that instead in future videos!
@@ArjanCodes I thought of exactly the same thing, but then remembered it's a 3.9 thing. If I may suggest, mention that some features are only available a subset of the currently-supported versions, as in the walrus operator, lest a newbie trips up while excitedly using it in an older version of Python.
Good idea, @Burner E!
yea it felt kinda awkward when he nested a typing.Tuple type inside a builtin dict type. for now I'll suggest stick with typing types only as 3.9 is pretty far from becoming the default still
another option would be to tell people to add the "from `__future__` import annotations" import at the top of their files, then it'll work everywhere from 3.7 and above, but still kinda cumbersome.
I love your videos!! Coming from a barely intermediate level knowledge of software design and few years experience coding, these videos showing code the whole time and putting the theoretical ideas in practice are fantastic! I prefer to watch your refactoring videos over TV any day! Thank you very much for the awesome content!
This channel is pure gem. Your way of explaining, speaking and use of examples as well as visualizations is simply awesome!
Thank you und keep it up!
Wow, thank you! And I will ;)
Legend!!! What a great content, presented in such an easy to follow along. And with a sense of humor! Huge fan of the work you are developing!
Thank you Marcello, glad you like the videos!
Excellent content, I think this is the only channel that I get excited when a new video appears. I’m looking forward to more of your podcast, left a 5 start review there as well!
Thank you Casey, glad you liked it!
Sick to see the progress your channel is making! Love the content, keep it up 💪🏻
Thank you Josh, glad you like the content!
Great content! Impatiently waiting for a video on design patterns for production-level machine learning projects.
Great suggestion, thank you!
+1
I will love to see a video regarding managing application configuration in Python, like taking configuration values from environment variables, text files, etc.
Marvellous videos. This is my favourite python channel.
Regarding the refactoring to put the conditions that must be satisfied first, and returning an error if they are not satisfied, I have heard that called ‘guard conditions’ which makes sense to me.
In python they may be a list of ‘try’ ‘except’ which if they all pass, then the function returns a value, if any of them fail, it returns an error or none or whatever.
Wow, your channel is growing like crazy. I remember view counts being around 150 just a few months ago. Great work!
Indeed! It’s been a crazy month!
The first programming CZcams who can actually code!!! Wtf! 😂😂 love the videos, keep it up, I’m learning a ton!
Found many points to be learnt from your video. Definitely a great supplement for my current course aiming to become machine learning engineer.
Glad to hear you found the video helpful!
This channel is a hidden gem for me as a data scientist who’d love to learn more about how to make my code cleaner and adhering to best practices. Thank you for the awesome content!
Glad to hear you like the content!
Thanks Arjan, you explain well. Goed bezig. I received your content through the Python Weekly news letter.
Dankjewel! Glad you liked it ;).
Hey man, I really appreciate your work with these videos. I would like to suggest a content about *args and **kwargs in the design patterns, since they can be used in class wrappers, are features utilized in big libraries (e.g., numpy, matplotlib) and barely has any documentation showing its use in real world development scenarios.
Thank you for the suggestion!
this is super cool content! would u consider making a session on how to write a good design doc?
Glad you like it and thanks for the suggestion!
I love this channel. When will you put out a tutorial on how to smile pleasantly? LOL Your outro smile is so friendly!
Thanks Scott. I’ll think about your suggestion ;).
Hey Arjan, fellow developer here, this is a very nice list of refactors and is cool to already have an idea on what you are going to suggest while you are describing the problem.
Keep it coming! It's very valuable content
Thanks so much, glad you liked it!
Awesome stuff Arjan!
Thanks so much!
Awesome video, I learnt a lot. Thanks mate
Thank you, glad you liked it!
Excellent content again. Great work.
Thank you, glad you like it!
I know comments are important for YT, but I can't help but say Thank you again. I know it takes a lot of work to produce at YT video and your hard work is much appricated! I installed pylance and black by the way.
Thank you very much, Sam. I’m happy the videos are helpful and I’m sure you’ll enjoy the combination of Pylance and Black.
these videos are awesome for improving code cleanliness. I have a doubt, I noticed that the line at 14:57 was very long, is it not recommended to use the 80 characters?
Excellent content, Arjan! Thanks a lot. I really appreciate if you make a video about SOLID principles using Python examples, can you make it?
Glad you like it! I did a video about SOLID principles a while ago: czcams.com/video/pTB30aXS77U/video.html.
interesting videos and got great ideas to improve my code. please keep adding such videos. it shows me how bad i am in coding :p
nice video, great job buddy🙂👏👏
Thank you so much!
I'm still a beginner, but your videos are gold! Thank you!
You’re most welcome! Happy you like it.
Video on when to use class methods would be great! Thank you!
Thank you for the suggestion!
Cool thing is that class methods can use other class methods or static methods
Very usefull! My favorite python teacher now :)
Thanks! 😊
I love your content because I targets the niche of people who know Python, aren't beginners and want to take their knowledge to a more polished level
Thanks Billy, that is exactly the type of audience I’m aiming the content for.
This content is so good!
Thank you so much!
Hi Arjan! I want to know your pov about the projects structure and how could be managed locally or within docker container. Thx you are making awesome videos!
Thank you for the suggestion!
@ArjanCodes - I do have question regarding your "Bonus". What is the purpose or benefit of enclosing main code inside a separate function ``main()`` which in turn is then called from inside the ``if __name__ == "__main__": ...``-condition; given that I otherwise could have just pasted its content directly into the code-block of the aforementioned condition?
The reason for using a separate main function is that this limits the scope of any variables you declare to that function body. If you put the code directly under the if statement, any variables you declare there are going to be available in the entire module (in this case, registry for example). This may lead to unexpected problems: shadowing warnings, or accidentally using a variable you didn’t intend to.
@@ArjanCodes but surely a major use case of that if statement is to ensure that your module's test code executes when you run the module as a top level script, but not when you import it from another script. If you are importing the file as a module, the if statement never executes and those variables never exist within the module namespace. On the other hand, if you're running the file to test the functionality then if your test codes tramples over your module namespace then probably you've got bigger problems to solve :-) I guess my attitude is that a main() function possibly indicates someone who is happier in C and then I start looking for Cmells in the code! I would agree that if you have real non-testing functionality inside that if statement then you probably want to put a lot of it into some functions, but I'm not sure I'd agree that main() is the best choice of name since it dopesn't have any specail meaning in Python unlike C.
Other than that quibble, I like you smells videos and know some grad students whp could usefully profit from watching them!
my biggest gripe with the python conditional oneliners is that the if is in the middle, while the normal python if is sturctures if condition: a else b
a
?thing_if_a_true
:thing_if_a_false
is so much easier to read and is still fine to read if nested
just watch your videos for fun but that fun making me more better
software developer
It's a good idea to import a class only to use it as a type of a variable?
these videos are absolutely fantastic, everything is so well articulated and you have so much knowledge (and wisdom) to impart, if you were to ever create more guides or even write a reference book I would buy it on the spot
Thank you so much! I’m working on a more complete design course at the moment, stay tuned!
Another good video. Can you do one on ABC? And your take on using them on OOP or another style you prefer
Thank you, and yes, I’ll certainly cover ABCs in more detail in the future.
@@ArjanCodes thanks! Looking forward for ABCs
Great stuff. Clean and concise. 1 suggestion. Your video is nicely paced as a whole. It comprises a number of 'chapters', one for each smell. You take corrective action for each one. If you held for a beat or two on the *correction* , I think it would be easier to understand, and have a better rhythm. I've noticed this tendency not to 'take a breath' in some of your other films? No criticism of your content - it's really helpful.
Hi Robin, glad you’re enjoying it! It’s a challenge to find the right pace on CZcams as in general videos need to move pretty fast here in order to not lose viewers. But I’ll surely experiment with this more.
Thanks for the great video.
One opinion about using return in the middle of the method:
It reminds me of using goto which we were told not to use. Imagine placing a breakpoint to debug the code and because of a return, it never gets there and can be confusing. A single return at the end of the method is probably a good idea.
I disagree.
As for why, its hard to explain in a comment!
But returning early once a condition is met (or error condition is met) is a better style for me. I guess it comes down to which is most ‘pythonic’.
15:27! WORD!! EXACTLY!!
I don't get this shortening/renaming obsession 🙈
I mean I've been there once ... but I genuinely like original module names much more now.
Subscribed ❤️
By using a stateful default value you introduced a new smell. Then by removing year=2021, you introduced a bug that proves why having a stateful default value is a bad idea (the launch year of those models will change every year).
Thanks for publishing such excellent content!
Question about the bonus code smell though. You mention that having variables declared in the if __name__ == '__main__' block could lead to name clashes and shadowing at the module level, but shouldn't that not be the case? If you import that module into another, the if block won't be executed.
Hi Curt, thanks! What I meant is name clashes within the module file itself. Any variables that you use directly under that if-statement are going to be accessible everywhere in the module file and that may lead to issues such as shadowing. Indeed, what’s under the if statement will only get executed if used as a main file. That means, you’ll have different scopes depending on the use case. If you accidentally use a variable defined under the if statement, the code will work when used as a main file, but not when used as a module (or in that case, the variable will be dynamically created leading to other possible bugs).
@@ArjanCodes ah, that makes sense. I misinterpreted what you were saying then. Thanks for clarifying!
cool video! thanks
Glad you liked it!
Hi Arjan, do you mind to share your black code formatting configurations?
I don’t think I changed all that much in the settings, except perhaps the minimum line length. I’ll do a video in the future covering extension settings and setup tips for VS Code.
Grea video, very insightful, thanks!
Ruff is also a nice tool to detect code smells. Any thoughts on that? :)
I consider myself a fairly capable hobbyist/opportunist programmer and this really helped me.
Thanks and happy you liked it!
I think for #4 i would have broken it all into if elif else. Swapping between if return and return if though much easier than before is still hard to read, compared to destinct cases and returns
Would love to see a video where you design a small system using a design tool of your choice, and then a follow up video where you implement it.
Thank you for the suggestion!
Phenomenal video.
Thank you so much!
i like to just import the module and then use the namespace.
import pwn
elf = pwn.ELF('/path/to/bin')
instead of
from pwn import a,b,c,d,e, ELF, and some more stuff i need
elf = ELF('/path/to/bin')
makes everything more clear, which code is using what library right there in the code.
Hi Arjan great video!
Maybe it's me being used to coding in C# and explicitly defining a type for everything, but something made me cringe when I saw you returned a tuple from a function... I would've probably made a dataclass or a named tuple to return the values. I try to be explicit when coding hopefully not leaving things up for interpretation, but hey I did say I come from a more statically typed language. I would like to know your thoughts on that. Thanks!
Hi Malcolm, thanks - glad you liked the video! I’m actually more experienced in statically typed languages as well (I coded in Java and then C# for over a decade). Python is a bit different. It relies more on duck typing and data structures such as tuples and dicts. So this is me trying to adapt to that world :).
Ok good to know your background. The way you wrangle python is very good... I guess that's why your videos are so popular and done really well.
I am trying to understand the duck typing thing... I think it will make a great video topic and you could also explain the pitfalls of allowing duck typing to happen.
I will add it as a discussion topic on discord.
My main struggle knowing which classes I need to create to solve a problem.
13:11 the dots that appear at line 110: is that pylint? 😃
I guess it's suggesting to remove the else and indentation because the code underneath wouldn't be evaluated after return anyway.
The else block still seems smelly: I'd make it return the connection error when there are no vehicle models and finally return ONLINE at the root.
I actually wanted to switch back to Java because of the way python was written. Hadn't seen your videos!!! Please make a tutorial on the way you are writing Python for the new ones or/ and anyone please link me a video that does that.
Hi Sir, I really like your videos , but nowdays since we use sqlalchemy alot. Could you please create a video on good practices while working with ORM classes and objects.🙏
what a guru thank u sensei
Thank you, glad you liked it!
Wow, never knew we can use a tuple as a key of dictionary! Cool!
Thanks!
Wildcard imports, my nemesis: I’m a physicist working on simulation and the most popular simulation code in my field uses them all the time! They use wildcards imports of all their modules, and in these modules most standard python libraries are also wildcard imported! Trying to figure out from where a certain function was actually imported is a nightmare. Also, they seem to be allergic to return function output, everything has to be created before calling a function and passed as an input parameter to have in place calculations performed on it.
Oh boy… that sounds like fun.
Using the "current" year as a default value is, in my opinion, usually a bad idea. 4:07
For example, on your code, you're creating all VehicleModelInfo instances without providing a year (4:48), which today might work perfectly fine, but next year it won't, since it'll then be 2022 instead of 2021. So basically, your code now has an "expiration date", after which it breaks, no longer works as intended, and will start failing tests.
You’re right. Perhaps a better choice for a default value would simply be ‘2021’ (int) in this case.
crushing it as usual (y)
Thanks Joshua, glad you liked it!
13:35 you don't need the else (guard clause)
You’re right - good catch!
Why is a lack of defaults a smell? Because there's so many parameters?
Yes, the smell is ‘too many parameters’. Defaults are a possible solution to that.
I love it
I tend to avoid and statements in conditional logic when one of the conditions has a higher likelihood of negating the need to check subsequent conditions that take longer to compute. But, I'm talking about millions of entries for NLP stuff.
I’m not sure but do most compilers/interpreters not optimize this such that if the first part of an and results in false, the rest is not even evaluated?
@@ArjanCodes Not sure. I assumed that the and waited for both results before computing a Boolean value. Is there an optimization in the machine language that simply returns False if, say, the leftmost horn of a conjunction returns False?
Not sure either. From pure efficiency of evaluating Boolean expressions it makes sense, but if those evaluations have side effects, this optimization breaks things (even though having side effects in conditions is a code smell imho).
I love these Smells Codes sessions. #arjancodes I tried to find the design/guide, I could not, seems to be a broken link
14:54 now this f-string line is uber convoluted and kinda hard to grasp whats going on. I'd always say: If there is more code in your formatting than formatting: give up the urge to inline and create short variables that you tuck into your string. You will also thank yourself whenever you step over that line when debugging.
17:58 ah I see :) 👍
This sum Quality vídeo right here
Thank you - glad you like it!
You should override the default __str__() and __repr__() implementations instead of writing your own to_string() function.
Checking for None without `is (not) None` is a bad practice imho.
I had the ‘is not None’ in the first version when I wrote the example, but then decided to remove it for brevity. Indeed, it might be better to leave it in as being explicit here is not such a bad thing. I think Go actually prohibits this kind of expression.
There are some code smells that are particular to certain people. For example, many coders love to smell ass. They also smell *like* ass. These two facts are loosely coupled. Those devs interface is usually open for modification
I hate python, but i watch your videos to learn programming tactics.
after you changed the list to dict you needed to delete the find_model_info because the dict do all the work to find the info
Nice.
Thanks!
I love the walrus/assigned expression operator but it's not universally supported. Cython won't compile with it and likely never will.
Good to know, thanks!
Aren't using an asterisk for the imports is eight code smell here.
I like the video in general but the walrus operator is what I would consider when I want to intentionally obfuscate my code 😂
Indeed, I also mentioned this in the video.
I’ve noticed a “code smell” in your example: it’s using len function to check whether a sequence is empty))
Why would that be a code smell?
@@ArjanCodes It was a joke)) It’s not actually a code smell, it’s just a nit pick.
No worries! But I’m curious: what would be your preferred solution?
@@ArjanCodes rather than “if len(collection) == 0” I prefer “if not collection”. Or instead of “if len(collection) > 0” I use “if collection”
There’s another comment on this same video suggesting the exact opposite and always make conditions explicit and not dependent on truthy values. I kind of agree with this. The Go language even forbids this kind of thing to force you to write conditions that clearly indicate what you mean. For example, ‘if collection’ could mean that you only want to do something if the variable is not None, or that you only want to do something if the list contains items. Assuming one or the other might lead to unexpected bugs.
Wow, this doesn't smell!
Using current year will lead to different operation depending on when you execute the code. Seems like a bug.
If you have 'if foo: return bar', then no else is need (cf. @13:14). This lets you remove one level of indentation.
I was thinking the same thing, when he suddenly started typing datetime. If the design requirements would explicitly call for that, okay. Otherwise this side-effect can lead to quite a data-mess that slowly starts snowing up around mid January 😈
Great channel, keep it going like this! The example and explainer code always feels like directly pulled from some company repo. Good depth and complexity, while still easy to grasp it in full.
Also your pacing is quite at the sweet spot to quickly get to the point, but leaves enough room to get ideas and opinions by yourself.
Oh and you fill quite a gap I experienced a while ago when starting with Python. Ido have quite some coding experience and wanted to go full-on "pythonic". Though I lack strong fundamentals in software design. During my journey I could often find just abstract docs about the pythonic and no real-life examples to guide my way.
Thanks you do a really good job in both regards!
Moi arrived 1st to process the stench of the snake script!
Enjoy 😉
Correction: stench created by those who don't know how to compose in the snake script.
The VehicleInfoMissingError should be raised by the find_vehicle method. You'd have less code and you'd get rid of a None, which is quite evil
Good suggestion. I had to make a few compromises in the example code to make sure all the smells fit it, this was one of them.
I love my code: Simple and Clean™
like 👍 & comment before starting the video 😁
Thanks! Hope it doesn’t disappoint 😊
The seven deadly code smells: Pride, Lust, Gluttony, Dopey, Sneezy, Bashful, Doc
droppin a like cuz i forsaw few smells and he almost convinced me he'd ignored them 😆
Haha, very good! :)
The else in line 112 at 13:30 is redundant
Correct, it’s not needed.
On line 79 you forgott to change the comment from saying "list" to saying "dictionary"... Thats why i always asume that comments are wrong. It even happens to the best people.
Code review: main() should be called run_main(). Functions are verbs!
In general I agree, but ‘main’ is such a standard entry point for an application that I prefer to keep that name for clarity.
When I see that 'stinky code', I understand that my code should smell like a bum)
I would hate for this guy to be reviewing my code
instead of tuple using concatination of brand+model as a key make more sense
Yea, wallrus operator, not really a fan of that...