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.
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!
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
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.
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.
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: ruclips.net/video/0mcP8ZpUR38/видео.html.
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.
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.
I so agree with the error handling. Builtin errors are for straight screwups breaking python. Custom errors are for things that break your abstractions.
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.
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
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.
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
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.
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.
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
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 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.
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.
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.
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.
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.
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.
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.
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
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.
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”.
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 :)
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 RUclips 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 RUclips 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.
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.
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!
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!
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.
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.
I myself don't like python ( but have to work with it ), but boy do I love some good explanation, and that code is just beautiful. I wish my co-workers could code stuff like that
The RUclips 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 ;-)
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.
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
Nice video. Thank you. Another improvement to the code is to not throw exception in the first place. Exceptions aren't part of the interface so it's not transparent to the client. Instead, you can ask the client pass in handlers when potential exceptions can occur - this gives client control over how the function works as opposed to forcing the client to handle business rule exceptions. Alternatively, you can pass back options (something or nothing/reason for nothing) and let the client handle it. My point is that it's not unexpected that vacation will either be paid or unpaid; therefore, why throw exception at all? Perhaps I made too much of the pedagogical example :). However, it is reasonable to throw exceptions at the context boundary where data comes into your context. Exceptions reflect how a user would treat bad inputs, IO error, etc as the user likely has a way to remedy the error at their level either by fixing the input or chasing down IO error.
You’re right - I would actually never write the code in this way either, but I needed to fit it into the example somehow, so that’s where it ended up 😊.
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.
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.
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
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.
Lots of great stuff here! One small comment: I've never seen returning None when a method doesn't return anything. I would just leave out the return type in that instance ...
I've thought about leaving it out, but decided against that, because this is more consistent: every method always has a return type. This also matches closely to how it's done in other languages, e.g. languages like Java or C++ also always require you to write 'void' as a return type.
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.
Great vid. I'll definitely have to utilize enums more; crazy how something as small as that makes it a lot neater. Love comprehensions as well but I stray away from them when it becomes too complex; readability, as you mentioned, is important. Anyway, very well done on your explanations man.
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
Good points! The code smell #3 isn't "Use built-in functions" - although it may be a good idea to look for already implemented functions for filtering purposes. However what you showed is to how to write more "Pythonic code". I saw no real benefit of inlining the whole for loop and it usually creates messier code. (Aka being a code ninja). I'd advise to use the 'fitler' function instead.
Maybe I can offer another solution to isinstance problem: Protocols. Protocols are interfaces in Python. Small example from docs _class Proto(Protocol):_ _def meth(self) -> int:_ ... _class C:_ _def meth(self) -> int:_ _return 0_ _def func(x: Proto) -> int:_ _return x.meth()_ _func(C()) # Passes static type check_
SOLID design principles help avoiding some of these code smells. Watch this video to cleanse yourself ;) : ruclips.net/video/pTB30aXS77U/видео.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 RUclips 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 😭
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!
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!
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.
This guy is a treasure, a real python expert on RUclips.
Thank you so much, glad you liked the videos!
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.
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!
The algorithm brought me here. Love the instruction and examples. Great work!
That’s really kind, thank you - happy that you like the video!
This video felt like a senior developer 1 on 1 reviewing my code, it was really helpful, thank you :)
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: ruclips.net/video/0mcP8ZpUR38/видео.html.
came here to comment exactly the same 😅
The custom Exceptions bit was actually really helpful for me! Thank you Arjan!
Glad to hear that!
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.
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™_
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.
I so agree with the error handling. Builtin errors are for straight screwups breaking python. Custom errors are for things that break your abstractions.
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!
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.
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!
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
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!
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
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...
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.
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
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
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.
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.
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.
Actionable, Entertaining and not alarming (get enough of that in the news). Thank you sir.
Definitely make this a series! Watching the code be cleaned is wonderful
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.
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.
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.
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.
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.
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.
Misschien wel de beste Python video's op YT..
Dankjewel, Rowan!
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
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?
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 😉
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”.
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!
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 :)
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 RUclips 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 RUclips 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
I absolutely love your videos, randomly found your channel and it is exactly what I was looking for. Exellent content.
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.
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.
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!
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!
Great job !! This a incredible reference for interns or freshers. Thanks for all the efforts.
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).
Love your style. Tempted to sign on to your course on API development.
Thank you Borge, hope to welcome you in the course!
Well edited, concise, clear and practical. Exactly what a tutorial should be
Thank you Jordan, glad you liked it.
I like how you're like
Good code => nice smell
Bad code => bad smell
Lmao
He's a smelly guy...
@@notinterested8452 lol
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.
I myself don't like python ( but have to work with it ), but boy do I love some good explanation, and that code is just beautiful. I wish my co-workers could code stuff like that
Glad you enjoyed it!
The RUclips 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 ;-)
Amazing. Need a video for building a better API with Python and maybe FAST API
Your understanding of Python is top notch!
Thank you so much!
I think I am strange, because I think these videos really entertaining. Great Content!
I feel personally roasted, yet this is the code review I needed.
Just learning python (I come from typescript and java) and this video just raised my hopes for the language 👌👌👌
And... You got a new subscriber!
Happy to hear you enjoyed the video!
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.
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
Nice video. Thank you. Another improvement to the code is to not throw exception in the first place. Exceptions aren't part of the interface so it's not transparent to the client. Instead, you can ask the client pass in handlers when potential exceptions can occur - this gives client control over how the function works as opposed to forcing the client to handle business rule exceptions. Alternatively, you can pass back options (something or nothing/reason for nothing) and let the client handle it. My point is that it's not unexpected that vacation will either be paid or unpaid; therefore, why throw exception at all? Perhaps I made too much of the pedagogical example :). However, it is reasonable to throw exceptions at the context boundary where data comes into your context. Exceptions reflect how a user would treat bad inputs, IO error, etc as the user likely has a way to remedy the error at their level either by fixing the input or chasing down IO error.
You’re right - I would actually never write the code in this way either, but I needed to fit it into the example somehow, so that’s where it ended up 😊.
@@ArjanCodes Thanks for the hard work. Cheers!
I’m so glad I discovered your channel! Amazing stuff, thank so much
Thanks - I'm happy that you like the videos!
Excellent video! I’m guiding my nephews learning and I will definitely share this with them. Brilliant and compact! 🎉
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.
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!
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.
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
I enjoyed this video! Very cleanly done and to the point.
Thank you, 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!
love the code smell series! hope there will be more in the future!
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.
I love your great practical advice on coding design. Thank you very much!
You're welcome and glad to hear you like the content!
Lots of great stuff here! One small comment: I've never seen returning None when a method doesn't return anything. I would just leave out the return type in that instance ...
I've thought about leaving it out, but decided against that, because this is more consistent: every method always has a return type. This also matches closely to how it's done in other languages, e.g. languages like Java or C++ also always require you to write 'void' as a return type.
@@ArjanCodes cool cool, i guess if we try to make it visually pleasing to Java / C++ users that makes sense.
nice quality! channel definitely deserves more subs
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.
The intro to this is superb!!!
Thank you 😊
I am hating youtube algorithm to be here so late. Such an amazing channel.
Thank you so much!
Amazing video! What a perfect way to learn about some new possibilities to improve my code. Really great!
Glad you enjoyed it!
Amazing video!!!
Sharing right now with my crew!
Thank you so much!
Great vid. I'll definitely have to utilize enums more; crazy how something as small as that makes it a lot neater. Love comprehensions as well but I stray away from them when it becomes too complex; readability, as you mentioned, is important. Anyway, very well done on your explanations man.
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!
Good points!
The code smell #3 isn't "Use built-in functions" - although it may be a good idea to look for already implemented functions for filtering purposes.
However what you showed is to how to write more "Pythonic code".
I saw no real benefit of inlining the whole for loop and it usually creates messier code. (Aka being a code ninja).
I'd advise to use the 'fitler' function instead.
Thanks for the feedback!
Maybe I can offer another solution to isinstance problem: Protocols. Protocols are interfaces in Python. Small example from docs
_class Proto(Protocol):_
_def meth(self) -> int:_
...
_class C:_
_def meth(self) -> int:_
_return 0_
_def func(x: Proto) -> int:_
_return x.meth()_
_func(C()) # Passes static type check_
Or use the new python match statements :)
Got this video in my recommendations! Very instructional, thanks!
Glad you liked it!