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
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.
@@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...
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"
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).
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 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.
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 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.
@@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 ;( ).
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
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!
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.
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.
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.
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!
Absolutely and good point. Actually, in my composition vs inheritance video, I’m doing exactly that, also with an employee example: ua-cam.com/video/0mcP8ZpUR38/v-deo.html.
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
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!
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.
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.
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.
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.
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
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.
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 ☺️
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.
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.
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.
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
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.
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!
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.
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
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
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.
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.
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 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.
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 :)
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!
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?
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.
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.
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.
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.
I so agree with the error handling. Builtin errors are for straight screwups breaking python. Custom errors are for things that break your abstractions.
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.
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.
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.
The UA-cam 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 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 ;-)
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.
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.
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!
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”.
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.
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
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
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
This video is excellent, both in terms of editing and content. Pointing something out as a constructive feedback. I found the thumbnail to be misleading. The display of the uncaught exception error almost made me avoid the video as I thought "oh, once again the same old mistakes that are pointed out in ever video". Instead, your video had no trivial content! You could choose something more original for the preview! Anyway, thanks for the content, great job! Subscribed!
It also contained incorrect information. First, `except Exception` will not catch a SyntaxError. Second, many many companies consider using built-in error types good practice. Why? You're reducing the amount of code you need to maintain. You'll also hear this from python core devs as well. Such as Raymond Hettinger and Jack Diederich.
This is such a great example of how to structure and think about code. A lot of people learned Python as a scripting language rather than a proper object-oriented language and just continue to try to use it that way. Some never learned C++ or Java, so have no clue about design principles. This is a really friendly way to breach the subject.
Not to throw anyone under the bus, I'm not naming anyone, but being a somewhat experiences programmers using languages like C, C++, C#, VB, I recently decided that I wanted to start learning Python. A language that I have had NO experience with. I watched the first tutorials that I ran across on UA-cam less than a month ago with a database project in mind which I've been using to learn with. I was using Python coding techniques which sort of bothered me a little but my ignorance of python which is still as high as mount Everest had me implementing these practices because they worked. I was bouncing back and forth between getting things working and errors I didn't understand which was a learning experience in itself. Long story short I was utilizing practices that are full of code smells. A concept not foreign to me in other languages. Luckily I ran across your UA-cam channel just a couple of days ago and I'm learning so much and of course refactoring my stinky garbage. Thank you for your 'higher level' contribution to the Python community. I'm learning a lot.
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.
Great Video! I see 2 problems with your find employee method: 1. You talk about using built ins to do things but don't use filter to filter a list. 2. Comparing roles with "is" is smelly imho. If somebody decides to deepcopy an employee for some reason the pointers will be different if your roles get more complex.
How is comparing roles with "is" a code smell? I don't think you understand what an Enum is in Python. Also, list comprehensions are genuinely seen as more Pythonic than the functional filter and map.
@@dhkatz_ I do understand enums, but in code like this, parts often increase in complexity. The enum could become its own class, or a person could have multiple roles, etc. I might have been a bit pedantic though. That depends on who you ask. In my experience a healthy ude of functional statements can make code much more readable and maintainable. Though, again I was a bit pedantic here. But wasn't being pedantic the point of the video? ;)
There's definitely a place for functional statements like filter, etc. Ultimately, whether you use list comprehension or functional statements depends on the use-case and what's the most readable. So the 'code smell' in this case would mainly be that you should choose the most readable and maintainable option (whether that's for loops, filters and maps, or list comprehension) and be aware of the possibilities that the language offers. Regarding the enum, changing the role from a simple value to indicate the type to a class instance with multiple attributes is a pretty significant conceptual change of what a role actually is. Of course it's very possible that it happens, but it will involve a major rewrite of the code in many places and you would surely also rewrite the comparison part. Comparing enum values by identity is the recommended way of doing it according to PEP 435: www.python.org/dev/peps/pep-0435/#comparisons. I don't think we should write code that doesn't follow the recommendations just because it's possible that sometime in the future we're going to redefine a concept and use it in a completely different way.
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?
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.
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.
Yes, I have! I’ve not focused to much on data science / machine learning topics at the moment, but I might do more videos on that topic if there is enough interest.
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.
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.
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.
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
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.
It's surprising to me to find a coding youtuber that's so entertaining and educational. I like how funny you are. Just one question: are you actually that fast when you type or are you just playing the parts where I hear you type in timelapse or something? It sounds like a machine gun.
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 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])
@@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/
As an experienced beginner/ bordering on intermediate Python programmer, it strikes me as strange that the very first smell detected actually requires importing a module to allow enumerated types. Seeing as they are a fairly standard concept, I always took their non existence in Python to indicate that they are deemed unnecessary in Python (a bit too "typey" perhaps). I generally get round similar issues with a series of Boolean options, although that does require some filtering to supress multiple selections.
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!
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.
I remember back in my first year of coding I can't figure out why a certain section of my code just wouldn't run for no apparent reason. The fact that the section that didn't want to run was a for loop didn't help at all, because I started off searching for any mistakes I've done on the list that the for loop is using. I stared and debugged my code for around 4 to 5 hours straight until I scrolled down long enough and took notice of the empty except statement I lazily wrote when I started that file. If only I watched this video back then lol
one code smell was introduced during this session. the 'pay' method does not belong to the 'employee' class. max of what 'employee' can do is to know how much needs to be paid. then, I would call a separate module to actually make a payment.
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.
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.
I agree that, in this case, the use of isinstance was bad. However, in one of my projects, I have an ABC which is further refined by a subclass (i.e., the subclass is also abstract). The subclass provides functionality not present in the base class. So, I have a function which takes an instance of the base class. It does stuff and then, if the instance is also an instance of the subclass, it invokes that extra functionality as well.
SOLID design principles help avoiding some of these code smells. Watch this video to cleanse yourself ;) : ua-cam.com/video/pTB30aXS77U/v-deo.html.
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
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.
Cleanse your :
@@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...
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"
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).
EXACTLY!
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. 😂
Doesn’t a decimal, or quadruple, still have rounding errors, but way less significant?
@@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.
@@andrew_ray thanks for this!
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.
Thank you so much, Diego, glad you enjoyed the video.
@@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.
This man explains stuff so calmly and I love his accent.
Thank you - I've been working on my accent a long time - and give it the right amount of "cheese".
@@ArjanCodes are you Dutch?
Yep! As Dutch as they come.
@@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 ;( ).
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
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!
Thank you - glad you liked it!
SMH I finally learned what enum does. This quickly turned from a code fragrance video into a level up the code video for me.
Glad it was helpful!
Same. Mind blown
And I learned that Python has enums. Didn’t know that somehow.
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.
Thanks, glad you like the content!
Honestly, the best python UA-cam channel out there for non-beginners! Incredible! Thank you for everything, I'm learning so much from you
I feel like if I'm not doing everything in these videos I'm still a beginner 😭
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.
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.
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!
Absolutely and good point. Actually, in my composition vs inheritance video, I’m doing exactly that, also with an employee example: ua-cam.com/video/0mcP8ZpUR38/v-deo.html.
came here to comment exactly the same 😅
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
Sped up? No man, those parts are actually slowed down 😉.
@@ArjanCodes I thought it was simply a good editor autocompletion!
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!
Glad to hear you’re finding the content helpful, Michael!
The custom Exceptions bit was actually really helpful for me! Thank you Arjan!
Glad to hear that!
I found you from reddit post and I'm glad I did, this video was superb. Amazing work!
Same
Thank you, glad you liked the video!
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.
I'm still new to Python programming and I just learned a ton in these 20 minutes!
Happy to hear that the video was helpful!
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.
This guy is a treasure, a real python expert on UA-cam.
Thank you so much, glad you liked the videos!
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.
The algorithm brought me here. Love the instruction and examples. Great work!
That’s really kind, thank you - happy that you like the video!
Everyone likes their own smell.
And code smell is just code flavour.
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.
If you let smelly code ripen enough, it eventually becomes _Code Umami™_
This video felt like a senior developer 1 on 1 reviewing my code, it was really helpful, thank you :)
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.
Deep lore
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
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.
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 ☺️
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.
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.
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.
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
Thank you for the suggestion!
@@ArjanCodes please do this series
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.
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!
Thank you for the suggestion! I'll make a note of this.
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.
Thank you, glad it was helpful!
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
Definitely make this a series! Watching the code be cleaned is wonderful
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
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.
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.
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.
I'm not sure what you mean by this. Could you explain further?
@@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.
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...
My favorite python instructor now. I've started watching all your videos. Your tutorial helped me improved my coding style.
Thanks, happy this is helpful to you.
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 :)
I absolutely love your videos, randomly found your channel and it is exactly what I was looking for. Exellent content.
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!
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?
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.
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.
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.
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.
Actionable, Entertaining and not alarming (get enough of that in the news). Thank you sir.
I so agree with the error handling. Builtin errors are for straight screwups breaking python. Custom errors are for things that break your abstractions.
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.
Great job !! This a incredible reference for interns or freshers. Thanks for all the efforts.
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.
Thanks! And you're right, they're not built-in functions, but they're built into the syntax.
This is the best python video I've ever watched. Loved the way you explain each and every change
Thank you, glad you enjoyed it!
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.
Thanks Pete, glad the video helped you.
Liked, subscribed, alerted, and commenting. This was amazing. The difference between debuggin with print("error here") and raising your own errors..
Thank you so much! Glad that you enjoyed the video.
The UA-cam 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 :)
Thank you Alexander, good to hear you enjoyed it, welcome onboard. Hopefully you'll find some extra value in some of my other videos ;).
@@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 ;-)
I like how you're like
Good code => nice smell
Bad code => bad smell
Lmao
He's a smelly guy...
@@notinterested8452 lol
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.
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.
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!
You’re most welcome!
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”.
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.
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).
I feel personally roasted, yet this is the code review I needed.
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
Thanks for the suggestion, I’ll put that on the list!
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
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
Love your style. Tempted to sign on to your course on API development.
Thank you Borge, hope to welcome you in the course!
This video is excellent, both in terms of editing and content.
Pointing something out as a constructive feedback. I found the thumbnail to be misleading. The display of the uncaught exception error almost made me avoid the video as I thought "oh, once again the same old mistakes that are pointed out in ever video".
Instead, your video had no trivial content! You could choose something more original for the preview!
Anyway, thanks for the content, great job! Subscribed!
It also contained incorrect information. First, `except Exception` will not catch a SyntaxError. Second, many many companies consider using built-in error types good practice. Why? You're reducing the amount of code you need to maintain.
You'll also hear this from python core devs as well. Such as Raymond Hettinger and Jack Diederich.
I think I am strange, because I think these videos really entertaining. Great Content!
This is such a great example of how to structure and think about code. A lot of people learned Python as a scripting language rather than a proper object-oriented language and just continue to try to use it that way. Some never learned C++ or Java, so have no clue about design principles. This is a really friendly way to breach the subject.
Thank you - glad you like it!
Not to throw anyone under the bus, I'm not naming anyone, but being a somewhat experiences programmers using languages like C, C++, C#, VB, I recently decided that I wanted to start learning Python. A language that I have had NO experience with.
I watched the first tutorials that I ran across on UA-cam less than a month ago with a database project in mind which I've been using to learn with. I was using Python coding techniques which sort of bothered me a little but my ignorance of python which is still as high as mount Everest had me implementing these practices because they worked. I was bouncing back and forth between getting things working and errors I didn't understand which was a learning experience in itself.
Long story short I was utilizing practices that are full of code smells. A concept not foreign to me in other languages.
Luckily I ran across your UA-cam channel just a couple of days ago and I'm learning so much and of course refactoring my stinky garbage.
Thank you for your 'higher level' contribution to the Python community. I'm learning a lot.
Thank you for sharing this! I really appreciate it :)
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.
Thanks Eric, hope the videos help you.
@@ArjanCodes this one did certainly
Great Video! I see 2 problems with your find employee method:
1. You talk about using built ins to do things but don't use filter to filter a list.
2. Comparing roles with "is" is smelly imho. If somebody decides to deepcopy an employee for some reason the pointers will be different if your roles get more complex.
How is comparing roles with "is" a code smell? I don't think you understand what an Enum is in Python.
Also, list comprehensions are genuinely seen as more Pythonic than the functional filter and map.
@@dhkatz_ I do understand enums, but in code like this, parts often increase in complexity. The enum could become its own class, or a person could have multiple roles, etc. I might have been a bit pedantic though.
That depends on who you ask. In my experience a healthy ude of functional statements can make code much more readable and maintainable. Though, again I was a bit pedantic here.
But wasn't being pedantic the point of the video? ;)
There's definitely a place for functional statements like filter, etc. Ultimately, whether you use list comprehension or functional statements depends on the use-case and what's the most readable. So the 'code smell' in this case would mainly be that you should choose the most readable and maintainable option (whether that's for loops, filters and maps, or list comprehension) and be aware of the possibilities that the language offers.
Regarding the enum, changing the role from a simple value to indicate the type to a class instance with multiple attributes is a pretty significant conceptual change of what a role actually is. Of course it's very possible that it happens, but it will involve a major rewrite of the code in many places and you would surely also rewrite the comparison part. Comparing enum values by identity is the recommended way of doing it according to PEP 435: www.python.org/dev/peps/pep-0435/#comparisons. I don't think we should write code that doesn't follow the recommendations just because it's possible that sometime in the future we're going to redefine a concept and use it in a completely different way.
Misschien wel de beste Python video's op YT..
Dankjewel, Rowan!
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?
Amazing. Need a video for building a better API with Python and maybe FAST API
Wonderful, clear and concise, only one problem.... I now have to go and refactor my project, no worries, only 10k lines or so.
Haha, thanks & I know the feeling 😉
Awesome. I agree with all 8 smells and I've been struggling with the errors which you have now enlightened for me.
Thank you 🙂
Thanks so much Barry, glad the content is helpful!
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.
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.
Brilliant, in terms of exceptions, what do you think of the "look before you leap" vs "ask for forgiveness" philosophies?
love the code smell series! hope there will be more in the future!
Great explanations as usual. Have you considered doing a video on good practices when working with pandas dataframes? Thanks!
Yes, I have! I’ve not focused to much on data science / machine learning topics at the moment, but I might do more videos on that topic if there is enough interest.
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.
Wouldnt a database provide the role options in real life? I believe that's where it belongs.
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.
Yes, I thought the same. Hard coding values that really belong in a database - wouldn’t that be considered a code smell?
I’m so glad I discovered your channel! Amazing stuff, thank so much
Thanks - I'm happy that you like the videos!
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.
Thank you Jakub, glad you liked it!
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.
Glad it was helpful!
I enjoyed this video! Very cleanly done and to the point.
Thank you, glad you liked it!
I am hating youtube algorithm to be here so late. Such an amazing channel.
Thank you so much!
Excellent video! I’m guiding my nephews learning and I will definitely share this with them. Brilliant and compact! 🎉
What is the shortcut that you used in 6:51? :)
Thank you for the video as well really amazing!
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
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.
It's surprising to me to find a coding youtuber that's so entertaining and educational. I like how funny you are.
Just one question: are you actually that fast when you type or are you just playing the parts where I hear you type in timelapse or something? It sounds like a machine gun.
Thank you! The fast typing is all Hollywood magic (in other words, I sped up the video to keep things interesting for the viewer).
19:58 how can i deal with same names?
I have code that uses tens of values:
self.value=value
Can it be prettified?
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.
@@THEMithrandir09 thanks for your reply. I was worrying about it, personally in some big parameters i use dataclasses
@@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])
@@jammydodger9288 thanks
@@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/
@6:50 What key combination did you use to replace all instances of "managers" with "employees" within the function??
Your understanding of Python is top notch!
Thank you so much!
This is second your video I have seen and this is really new level for me, thanks for your job!
Thank you Alex, glad to here you like the videos.
As an experienced beginner/ bordering on intermediate Python programmer, it strikes me as strange that the very first smell detected actually requires importing a module to allow enumerated types. Seeing as they are a fairly standard concept, I always took their non existence in Python to indicate that they are deemed unnecessary in Python (a bit too "typey" perhaps). I generally get round similar issues with a series of Boolean options, although that does require some filtering to supress multiple selections.
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!
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.
I remember back in my first year of coding I can't figure out why a certain section of my code just wouldn't run for no apparent reason. The fact that the section that didn't want to run was a for loop didn't help at all, because I started off searching for any mistakes I've done on the list that the for loop is using. I stared and debugged my code for around 4 to 5 hours straight until I scrolled down long enough and took notice of the empty except statement I lazily wrote when I started that file. If only I watched this video back then lol
one code smell was introduced during this session. the 'pay' method does not belong to the 'employee' class. max of what 'employee' can do is to know how much needs to be paid. then, I would call a separate module to actually make a payment.
The intro to this is superb!!!
Thank you 😊
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.
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.
Looks like a live coding session replay. Great video!
Thank you!
I agree that, in this case, the use of isinstance was bad. However, in one of my projects, I have an ABC which is further refined by a subclass (i.e., the subclass is also abstract). The subclass provides functionality not present in the base class. So, I have a function which takes an instance of the base class. It does stuff and then, if the instance is also an instance of the subclass, it invokes that extra functionality as well.
Can you explain why super() was needed in your custom exception?