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.
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!
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.
@@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" :)
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
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.
I will love to see a video regarding managing application configuration in Python, like taking configuration values from environment variables, text files, etc.
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 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.
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.
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!
@@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.
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!
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!
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 RU-vid as in general videos need to move pretty fast here in order to not lose viewers. But I’ll surely experiment with this more.
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
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!
9:55 I think walrus operator is a code smell in 99% of the time. just because something can be done in one line, it doesn't necessarily have to be done in one line.
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?
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.
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’.
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
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).
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.
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.
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
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.
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.
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.
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.
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).
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.
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 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.
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.🙏
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).
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.
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.
I think people who complain about code smell are people who refuse to work on difficult problems. The point of programming is to solve problems, not write the most perfect, elegant code.
I think people who view programming purely as a tool to solve a problem are not suitable to work on difficult problems. Difficult problems require preparation, a good foundation and ability to adapt the software as the needs/requirements change (which happens continuously). The crucial aspect of creating this foundation is proper software design and fixing code smells to avoid bigger problems in the future.
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!
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.
1: No default values and too many parameters 2: Hadouken code 3: Wrong data structures 4: Too nested conditional expressions 5: wildcard imports or from... instead of simple import (unless it's really common and you will use it all over the place) 6: Doing the same thing in different ways 7: Passing self as a method parameter when it's not used in the code instead of turning it into a static method 8: Not using a main() function
@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!
