The time and energy you spend on reading the code, recording the video and editing it deserves respect and is much appreciated. Great amount of useful information!
22:05 calculating a straight can be done even simpler requiring no fancy math. 1. if has duplicates return false 2. else return max(l) - min(l) == len(l)-1
I would have used sets, likely; that seems closest to what the rule is about intuitively, more self-documenting. For large straight, check if set(l) in ({1,2,3,4,5}, {2,3,4,5,6}): ... or if you want it to generalize to the small straight more easily l = set(l) if {1,2,3,4,5}
Is it not just set(l) == set(range(int(min(l)), int(min(l)) + len(l)))? (this is also more explicit, saying exactly which numbers should be in the hand)
All of these methods don't properly allow for abstracting to longer lists of dice. The proper method is actually a loop that runs through each element in a list, resetting to the value it sees when it sees a jump up by more than one, and returning true when it sees a difference between the remembered value and the current one equal to the strait length, and returning false if it exits the loop. This way you don't need to try all subsets of the dice, you only need them to be in sorted order.
Very nice refactoring video which shows not only the refactoring of a single class or method, like most other videos do, but a full application. I like how you explained your thought process throughout the process. Keep up the good videos :)
Such great videos. I appreciate the editing and recording in high definition so I can get the full effect on a big monitor. Also the jokes and captions are great as well. Thank you, please continue!
One little thing, perhaps you already know, but CON-sole is pronounced like so when talking about computer consoles or gaming consoles, but pronounced con-SOLE when it is used to mean "troosten/vertroosten". Once again, amazing video, I just noticed this at one point where you said con-SOLE (around 5:55) multiple times in a single minute. Thought you might like to know, is all :)
This is incredibly valuable. It’s so very well explained as to why these changes are made. Subscribed for this series and your insightful Code Smell videos.
Wow, code roast is a great idea. I'd like to see more of these! I write python code professionally. I love Uncle Bob's "Clean Code" book, but I still need to see real-life examples of SOLID in action so that it's more intuitive for me to incorporate in my code. For example, I *loved* that you said WHY this person might want to decouple the print statements from the game logic. What if they want a GUI version of the game? My reaction to that is "you're right! My app is obviously going to be famous and decoupling seems like a great way to make it easy to add more display methods later on". If only you could help me refactor my React app :D
Hi Eric, glad to hear you liked it! Indeed, the why is really important when talking about design principles and patterns. I'll surely do another one of these code roasts in the future, so stay tuned ;).
Thanks for this helpful video! Could you please explain, why do we need to inherit all the Aces-Sixes classes from the SameValueRule class rather than let them be just instances of SaveValueRule with their particular values/names?
Wow... I can't even begin to explain how much I liked this video. I need to watch this about 5 more times. The editing is great. The pace is great(for me). The dialogue is fantastic. Arjan, if you're reading this, definitely keep up this series. This is probably the best python refactoring video on youtube.
Awesome, I was going to refactor a card game I madebecause I want to add AI and gui with Arcade. This videos had some nice takaways. 16:10 you can actually specifiy at which integer the enumerate function starts counting: enumerate(self.hand, start=1)
Great video, as usual--I always learn a lot from your lessons. But I did notice a possible improvement: At 21:28, you show a clever method for determining if a hand is a straight. However, I think this is simpler, and avoids the reader's need to understand that math about consecutive sums. Replace lines 76-78 with return (max(l) - min(l) == len(l) -1) Edit: I just read the other comments...I'm not the first to mention this. 🙂
Thank you for the video. There are a lot ideas and thought that are worth thinking about them. But I have a "suspicious mind" ;-): Does the method is_straight computes a correct result? E.g. with the hand 1,2,2,3,4 the method would return False. It's because the unique element set/list has length 4 und the original list has length 5. As far as I know the Yahtzee rules, a small straight only needs to have 4 consecutive values and a dublicate value is allowed. Or, did I miss something?
I hope you do more of these on occasion! Especially how you work on the whole program and not just a piece of it. Usually I get stuck at the "I know this is not 'good' code" stage, but I don't often know what to do about it to fix it yet. So it's extremely helpful to see the way you analyze and rethink things.
One point I'd like to push back on is your super fancy straight detection system. This is a yahtzee game on a modern computer. It's not running millions of times per second, it's not in a lambda function where every ms counts and its not on an old slow processor. So readability (explicitness) is more important than optimization. Large straight: sorted(x) == [1,2,3,4,5] or sorted(x) == [2,3,4,5,6] I hope i got my slice syntax right, its been a while Small straight: any(x == sorted(hand)[1:] or x == sorted(hand)[:6] for x in ([1,2,3,4],[2,3,4,5],[3,4,5,6]))
With all due respect to @Clear Code, would you consider roasting the following code? He would need to consent of course. ruclips.net/p/PL8ui5HK3oSiGXM2Pc2DahNu1xXBf7WQh-? The goal is not to trash him. His videos are wonderful! However, there are parts of his code which scream out for refactoring.
Isn't this version of Yahtzee allowing you to input a score of 0 into your scoreboard and then replay it? i.e. i play until i get carré, yahtzee, large straight etc, if i don't get any of them, i put the score in the Aces, Twos .. category that gives me a 0. Then i can reuse that slot the next time as the check is only if the score > 0.
I noticed that too. Can be solved by initializing scores with a sentinel value like -1 that signal the still-allowed rules. (Although then the total score needs to sum over max(points[idx], 0).)
Really great content Arjan! Love your videos. I was wondering if it would have been possible to refactor the code with the red-green-refactor TDD style, even though there weren't enough tests in the beginning? You spend like a couple hours refactoring before running the tests again.
Very nice video! I enjoy your videos, it helps going back to the basics from time to time :) One slight nitpick... you create methods and parameters to better test the code. In my opinion code should be agnostic of tests, since every additional "moving part" complicates the code, and more complex code means more chances for bugs. Also- the code does not need these methods (the tests do), so YAGNI applies. Having those methods like setting the faces of dices and hands in the tests on the other hand make perfect sense- they should be convenience methods of the tests, in my view.
Hi Dirk, glad you like the videos. Regarding the test helper methods: good point and I fully agree with you. Those methods are indeed better placed together with the tests to not 'contaminate' the original code.
@@ArjanCodes Arjan, would it not make sense to make these methods you use for testing somehow private? Maybe with "_" or "__"? Or... maybe you could dynamically attach a method of convenience right in the testing code? How would you do it?
Very cool refactoring! Couple of things that made me double back: - Die.set_face() can set values outside of Die.sides - 41:22-41:32 the error in SameValueRule.points() is still shown, your demo-run used some other codebase? You used enumerate() in some places without using the idx to index into your list - instead simply printing idx + 1 - could be simplified to enumerate(self.hand, 1) and remove the idx+1
Thanks - good points! I had to cut a few parts of the video, otherwise it would have become even longer, so there are a few places where an error is still shown. I did put the complete refactored code in GitHub which contains all the changes.
Your videos are so great. Always straight to the point and so well presented with a calm and pleasant voice. Btw, your clear pronunciation makes it very pleasant to listen to, even for non-native speakers.
Arjan, I reeeeally like your videos! It has helped me structure my code so much more clearly! It is making a huge difference in my python learning journey! I would really appreciate it if you could roast one of my projects hahahaha keep the amazing work!!
I very much appreciate the idea behind this video and the hard work that went into it. Me personally, I would not mind watching an even longer video just to your actual process of refactoring, not only the end result. Maybe let it be a multi part serious.
31:32 and prior where you define the rules for Aces, etc - you are assuming the dice are 6-sided and you still have duplicated code for Aces, Twos, Threes, etc. Should use a data structure to map 1:’Aces’, etc maybe look for a number to english package and use that but change “One” to Ace”. Other than that - VERY NICE!!
Holy hell, this is the most useful refactoring tutorial/coding tips/good practices video I've ever seen. Incredible work. I will continue to watch these in full for as long as you make them. Please continue making them!
You could even refine the combination tests by using combinations from itertools and counting all not repeating/permuting pairs (only once). 2 would be two_pairs, 3 three_of_a_kind, 4 full_house, 6 four_of_a_kind, and 10 would be the Yahtzee. Of course the Fibonyahtzee cannot be tested this way. I think, it should be worth only 89 points (or 144).
Great video, all your content is amazing! The ammount of effort you put into creating the examples, shooting and editing is astounding. Tho the "thats what she said" killed me lol.
@9:00 Adding code to make testing easier is an AWFUL idea. You're concerned with code smells? That reeks like a rotten corpse! 1. You're forcing test-only code into production. 2. You're adding functionality not indicated in the project requirements. 3. To have 100% test coverage, you have to have a test that tests your test code. 4. You're not testing production functionality. You're testing that your code works if you use an undocumented feature. 5. You're adding potential security risks. And for what? To make testing convenient? Prettier? It's testing. Who cares? Convenient? It's a cut and paste. If you want helper methods, put them in your test suite and don't make them visible to production code. Need to test times, or random values? Pass generators in production then mock them in test. Don't do this. Don't allow this. Make sure this fails a code review.
One of the few times where I get a reminder to press like and I'm like "yeah, I definitely need to do that", considering I have spent hours going through the original already so I could be ready for your notes and refactoring strategies. Great video. Are you still accepting submissions for refactoring videos?
3 года назад+2
I quite enjoyed the video. My nitpick would be is that the refactored code is not very pythonic. I mean, it does not take advantage of the data model (dunder medhods) and the standard library. I find that clases adhere to the data model (implement certain dunder methods to emulate the behaviour of some of the built in data structures) are easier to use. For example, if we think that a Die is an integer (with range restrictions and a rolle method) and that a Hand a list of Die, then we end up with sum(hand) instead of hand.sum() and die == 5 instead of die.get_face() == 5, which I find easier to read. I suspect that could be achieved by reaming Die.get_face to Die.__int__ and Hand.sum to Hand.__sum__ and maybe implementing Die.__eq__ @ArjanCodes, would you mind a PR with more details?
Thanks! When I posted this video, I was not yet fully committed to Python on my channel, as I am now. Since then I've started adhering to a more Pythonic standard for the code examples. I agree that here it would be nice to rely more on dunder methods to make the classes easier to use. I'll make sure to include that in follow-up examples/roasts.
3 года назад
@@ArjanCodes maybe you can roast your own code? I'm looking forward to the next in the series. BTW, had quite some funn today refactoring your advanced version. So far have only done the hand and rules module. There is a bug on the small straight rule, the 1st clause (len==4) is unreachable... 100% coverage might not not the end goal, but +90% turns out to be quite useful.
I like your videos very much! I have a small problem with your implementation of the scoreclass. Just checking if a frow has value 0 can lead to the fact, that I can write a row more than one time. I think there should be something like is_row_writable, so you can set a value just once.
I just wonder why we don't learn programming in the right (or more desirable) way from the very beginning? I made many tutorials here on RUclips and many of the programmers didn't use SOLID principles. No-one mentioned the clean code and TDD concepts. I have the impression that specially in embedded applications using Raspberry Pi, Esp-32 , Raspberry Pi Pico, Arduino, PIC or Stm32 the code isn't structured in that very clean and understandable way. Even official libraries used in for the mentioned hardware. I repeat: I just can't understand why is that so. I have heard that even in some university courses there are professors who seem not to follow those ideas and pass them on. Why???
I don't understand why one would need extremely simple one-line functions as in 24:13 where you defined rule_count which simply returns the number of rules. Why wouldn't the coder just write len(self.rules) instead of using this function?
I have no fucking idea of most of the things you are doing or talking about, but I think I'm learning much more in this 45 min video than in 10 hours of python tutorials.
I'm really enjoying your content and learning a lot👍 Quick question. You check for an empty string using len, as I do. But people always tell me to test as a Boolean. I think len is more readable and distinguishes None. What's your thinking?
Checking if an object is falsy/truthy by just writing "if obj:..." is faster (especially for the builtin types) and, as some say, more pythonic. There is a vid on YT (but can't remember right now the title) where a Python developer talks about it and presents statistics.
25:30 is it ok to use a plain `Exception`? As `except Exception:` is severely frowned upon aren't you raising an exception which can only crash the program?
These videos are really inspiring me to finally get around to refactoring my Discord bot implementing a multiplayer game with secret information. I got it to a point of basically functional last year, but the design is rife with the sort of code smell you're tackling in these vids, and I've found making adjustments to the original code very cumbersome. I particularly enjoyed this real-life example, giving me a wider variety of ideas and new perspectives. If I were to make any requests, it would be (1) logging best practices and (2) how you test your code if it relies on data models from an external API. In my example, I use discord.py's User model for getting names and sending PMs, as well as the Guild model for sending public messages. Making dummy instances of these classes to test offline got so difficult/unreliable that I've really only tested by manually putting it together on Discord as if I were playing the game with friends. And since it's a multiplayer game (min 5 players, max 10), I coded a bunch of "echobots" that respond to different command prefixes to simulate my extra players. Imagine me sitting here sending "0-echo !approve; 1-echo !reject; 2-echo !approve..." Very messy solution that has bogged down my development process quite a bit.
I think you skipped over some details with the explanation of computing the consecutive sum: the shown formula is n*(n-1)/2, but the implementation is (max+min)*(max-min-1)/2, i.e. max+min = max-min which is true only for min=0.
Such a useful video and thank you for spending your time going through everything in detail! There's definitely not enough of these videos around, so this is great!
while the refactoring thought process was quite enjoyable to watch, there are a couple issues with the final code with respect to the actual rules of the game, i think. the N of a kind for 1..6 value is *just* the sum of the faces of 1..6, not the whole hand. the small straight should be the length of the set(sorted(l)) as a start... then have to check the len=4 for missing internal # in some fashion, and the sum test for == 10 (1..4) or 18 (3..6) would indicate a "bottom" straight or a "top" straight". sadly, the result for an 'inner" striaight, "14", is aliased, so need to check for lowest ==2 and highest ==5. there is the concept of lots of good stuff for *multiple yahtzees*, which there is no hint of in the original code (you gotta work with what you got, i guess :D) there are a couple bonuses that accrue at the end of the game, the "top half" of the sheet gives 35 for a sum of >= 63 in all the top categories (achieved by banking 3 of each, e.g.) and the "bottom half" has multiple yahtzee extra points. might want to reconsider the commercial aspiration of this! ;)
One thing I'm not so comfortable with this is how long you spent with broken code. I would have preferred to do this with several small intermediate refactorings and always make sure the code still works as expected.
Normally this is not how I work. I agree that small refactorings are better, unfortunately that doesn’t work in a video like this as it would become way too long (and this video already was extremely long).
Truly excellent video on separation of concerns and refactoring Arjan :) - A video suggestion I would love to see is the analysis on how you arrived at this structure for your classes and general architecture. Do you use UML or do you analyse the requirements and create classes based off of the SOLID principle. Any help with how to start a project or refactor existing ones with this in mind would be invaluable!
You can definitely use that instead. I like to be explicit in conditional expressions and not rely on truthiness too much as it might not be clear in what cases you expect the condition to hold, leading to potential bugs.
can I submit a shell that I made in python ? it does a lot of stuff but since I made it before learning about software design, it is really badly implemented
Not gonna lie, this was a pleasant video to watch. It’s not often that you see how other people conduct code reviews and that is valuable. Subscribed ❤️
I've been a web developer for the last eight years and your videos have some of the best clear advice for application architecture I've come across. The relaxed atmosphere and high-quality of the recordings make a huge difference as well! You really know your stuff. I'm watching your videos because I'm intending to switch from PHP web development to Python web development. I am really trying to internalize the Pythonic approach to coding, as opposed to just coding PHP-style in Python. These videos are helping me a lot, especially presented in the context of application design which is familiar to me from PHP MVCs. I know this video is a year old already, but something in here has got me thinking on a code style topic that comes up for me a lot working with other developers in production contexts. That's the use of abbreviations in variable names. In this video you use the abbreviation `strs` for a List of strings in the `create_points_overview` method. Then you introduce a "typo"-category bug where you are referencing singular `str` instead of plural `strs` and you don't catch it until later. Thanks for including these kinds of bugs in your video and not editing them out. Bugs of this kind are a regular part of coding. I think developers of all experience levels encounter these "typo" kind of bugs and being aware of just how common they are is a great step towards having a coherent approach towards dealing with them.. Robert C. Martin makes what I think is a strong argument against the use of abbreviations for variable names in his Clean Code book. In my subjective point-of-view, without abbreviating the word `strings`, it would have been clearer that the variable name needed an `s` on the end. Even better, if the variable were to have a descriptive name like `point_rows`, then reading the code could become more conversational and it's easier to catch typos, especially of the plural/singular variety. My argument, lifted from Martin, goes like this: a descriptive approach to variable naming that aims for high specificity and doesn't use abbreviation can reduce the frequency of uncaught "typo"-category bugs in your code. Making code more comprehensible has additional value besides as well. This is all subjective, but I haven't heard a great argument *against* this approach. Variable name abbreviation was ingrained in the learning docs for major languages like Java, so it's not really any wonder that is so common. Martin's non-abbreviated variable naming approach based on my subjective experience feels more common in the PHP world than it does in the Python world. Clean Code also suggests eliminating almost all code comments, which I am influenced by and I have reduced my code comments, though I still use code comments frequently. Any thoughts on the influence (or lack thereof) of the Clean Code approach in the Python community, of the use of abbreviations in variable naming, and variable naming in general? Might make a good video topic. Thanks for the excellent content!
Hi Stephen, thanks so much for sharing this! I really appreciate your message, and I'm glad that my content is valued so highly. Your suggestion is noted! ❤
I'm kinda confused on your stance on "If you ain't gonna use it, don't implement it". You still separated view (print) from logic because we might publish it for some other device (iOS/Android/Web), but we know that we ain't gonna do it, so why refactor it for that possibility?
I think it helps to make a distinction between the overall design of a software application and specific features that you're adding to it. For specific features, like adding a JSON export format to store the state of the game, only do that if you really need it, because building and testing features takes time that you can potentially invest elsewhere. For the overall software design, it's crucial to design with separation in mind, because a) extending the system at a later stage is easier and b) you want to be able to test your software easily. In that sense, "if you ain't gonna use it" doesn't apply here. Having a more generic design is something that you actually WILL use, because it will save you time when you write your tests as well as when maintaining the software.
He means don't implement methods and features you don't use. The refactoring here addresses design flaws.. you should design the code for scalability, portability, reusability and testing. There are a lot of things to consider when writing code. Because generally speaking software changes all the time.. new requirements come and you need to design your code for change..
This content is beyond gold. And this "refactoring code" seres is an awesome idea!! Here home we love your work and appreciate the work you put into it.
In hand.py, line 16 why not use the any() function instead of liste comprehension? This way the if statement would be triggered on the first violating value instead of computing the entire list, then checking it's length. Also in the constructor why not use a list comprehension to initialise self.dice? Loving your work :)
Hi, thanks and good suggestions! I did a lot of refactoring in this video (a bit too much for one video I realized later), which led to less optimal solutions in some cases. If I do a code roast again, I’ll probably do a shorter one and spend more time on finding the best way of rewriting the code.
This is easily one of the best programming channels out there. Your explanations are always very clear and easy to follow. As someone who just programs as a hobby and has no formal training, my codes used to be a complete mess, everything in one big smelly file and it was a nightmare to debug and change something. Just trying to implement better structure and some of your practises has improved my quality of life as an amateur coder a lot.
@ArjanCodes It's a very cool concept! I have a question, I have noticed that you are using methods when properties could have done the same thing, is there a reason why you didn't use properties in these cases?
Thanks Kevin, there's no particular reason for using methods actually. Properties are a great option, but I forgot they existed in the heat of the moment ;).
Pretty long video this time! Now it's time for Yahtzee & chill ;).
loving this format. please do more
Great format please keep them coming
Please continue great work
The time and energy you spend on reading the code, recording the video and editing it deserves respect and is much appreciated. Great amount of useful information!
Hi Davoud, I'm happy you're enjoying the videos and that they're helpful!
22:05 calculating a straight can be done even simpler requiring no fancy math.
1. if has duplicates return false
2. else return max(l) - min(l) == len(l)-1
Ha! You’re absolutely right. Sometimes you think of a really complicated solution when there’s a much simpler approach possible 😊.
I would have used sets, likely; that seems closest to what the rule is about intuitively, more self-documenting.
For large straight, check
if set(l) in ({1,2,3,4,5}, {2,3,4,5,6}): ...
or if you want it to generalize to the small straight more easily
l = set(l)
if {1,2,3,4,5}
Is it not just set(l) == set(range(int(min(l)), int(min(l)) + len(l)))? (this is also more explicit, saying exactly which numbers should be in the hand)
All of these methods don't properly allow for abstracting to longer lists of dice. The proper method is actually a loop that runs through each element in a list, resetting to the value it sees when it sees a jump up by more than one, and returning true when it sees a difference between the remembered value and the current one equal to the strait length, and returning false if it exits the loop. This way you don't need to try all subsets of the dice, you only need them to be in sorted order.
Very nice refactoring video which shows not only the refactoring of a single class or method, like most other videos do, but a full application.
I like how you explained your thought process throughout the process. Keep up the good videos :)
Glad you enjoyed it Florian! And will do ;).
This channel is so underrated, I love the examples, the explanations, everything. Awesome work!
Thank you so much Diego!
I have learned so much from this channel - thank you for all the amazing content!
Thank you Adam, glad you like the videos.
Time to send his own git repo code for roast 🍺
Haha, I’m all up for it 😊
Such great videos. I appreciate the editing and recording in high definition so I can get the full effect on a big monitor. Also the jokes and captions are great as well. Thank you, please continue!
Thanks, happy you like it!
I enjoy seeing you go through code.
Thank you! Glad you enjoy the videos.
This is... simply amazing!
I've just discovered your channel. Bravo. Great video. Wonderful detailed explanations and reasoning for making changes.
One little thing, perhaps you already know, but CON-sole is pronounced like so when talking about computer consoles or gaming consoles, but pronounced con-SOLE when it is used to mean "troosten/vertroosten".
Once again, amazing video, I just noticed this at one point where you said con-SOLE (around 5:55) multiple times in a single minute. Thought you might like to know, is all :)
This is incredibly valuable. It’s so very well explained as to why these changes are made. Subscribed for this series and your insightful Code Smell videos.
Glad to hear it!
Wow, code roast is a great idea. I'd like to see more of these! I write python code professionally.
I love Uncle Bob's "Clean Code" book, but I still need to see real-life examples of SOLID in action so that it's more intuitive for me to incorporate in my code. For example, I *loved* that you said WHY this person might want to decouple the print statements from the game logic. What if they want a GUI version of the game? My reaction to that is "you're right! My app is obviously going to be famous and decoupling seems like a great way to make it easy to add more display methods later on".
If only you could help me refactor my React app :D
Hi Eric, glad to hear you liked it! Indeed, the why is really important when talking about design principles and patterns. I'll surely do another one of these code roasts in the future, so stay tuned ;).
Thanks for this helpful video!
Could you please explain, why do we need to inherit all the Aces-Sixes classes from the SameValueRule class rather than let them be just instances of SaveValueRule with their particular values/names?
Wow... I can't even begin to explain how much I liked this video. I need to watch this about 5 more times. The editing is great. The pace is great(for me). The dialogue is fantastic.
Arjan, if you're reading this, definitely keep up this series.
This is probably the best python refactoring video on youtube.
Thank you so much, I’m happy that the video is helpful to you. I’ll definitely do more code roasts in the future.
Awesome, I was going to refactor a card game I madebecause I want to add AI and gui with Arcade. This videos had some nice takaways.
16:10 you can actually specifiy at which integer the enumerate function starts counting: enumerate(self.hand, start=1)
Thanks! And good tip about the enumerate function - will keep that in mind!
Great video, as usual--I always learn a lot from your lessons. But I did notice a possible improvement:
At 21:28, you show a clever method for determining if a hand is a straight. However, I think this is simpler, and avoids the reader's need to understand that math about consecutive sums.
Replace lines 76-78 with
return (max(l) - min(l) == len(l) -1)
Edit: I just read the other comments...I'm not the first to mention this. 🙂
Thank you for the video. There are a lot ideas and thought that are worth thinking about them. But I have a "suspicious mind" ;-): Does the method is_straight computes a correct result? E.g. with the hand 1,2,2,3,4 the method would return False. It's because the unique element set/list has length 4 und the original list has length 5. As far as I know the Yahtzee rules, a small straight only needs to have 4 consecutive values and a dublicate value is allowed. Or, did I miss something?
I hope you do more of these on occasion! Especially how you work on the whole program and not just a piece of it. Usually I get stuck at the "I know this is not 'good' code" stage, but I don't often know what to do about it to fix it yet. So it's extremely helpful to see the way you analyze and rethink things.
I'm happy you liked the video - I think I'll do another one soon. I really enjoyed filming this video, so more is coming ;).
One point I'd like to push back on is your super fancy straight detection system. This is a yahtzee game on a modern computer. It's not running millions of times per second, it's not in a lambda function where every ms counts and its not on an old slow processor. So readability (explicitness) is more important than optimization.
Large straight: sorted(x) == [1,2,3,4,5] or sorted(x) == [2,3,4,5,6]
I hope i got my slice syntax right, its been a while
Small straight: any(x == sorted(hand)[1:] or x == sorted(hand)[:6] for x in ([1,2,3,4],[2,3,4,5],[3,4,5,6]))
Haha, yes, sometimes I go a bit overboard :). Good alternatives!
With all due respect to @Clear Code, would you consider roasting the following code? He would need to consent of course.
ruclips.net/p/PL8ui5HK3oSiGXM2Pc2DahNu1xXBf7WQh-?
The goal is not to trash him. His videos are wonderful! However, there are parts of his code which scream out for refactoring.
Isn't this version of Yahtzee allowing you to input a score of 0 into your scoreboard and then replay it?
i.e. i play until i get carré, yahtzee, large straight etc, if i don't get any of them, i put the score in the Aces, Twos .. category that gives me a 0.
Then i can reuse that slot the next time as the check is only if the score > 0.
I noticed that too.
Can be solved by initializing scores with a sentinel value like -1 that signal the still-allowed rules.
(Although then the total score needs to sum over max(points[idx], 0).)
At 22:05, do you need to convert the set into a list to find the length? Can't you just do "len(set(l)) != len(l)"
Really great content Arjan! Love your videos.
I was wondering if it would have been possible to refactor the code with the red-green-refactor TDD style, even though there weren't enough tests in the beginning?
You spend like a couple hours refactoring before running the tests again.
Very nice video!
I enjoy your videos, it helps going back to the basics from time to time :)
One slight nitpick... you create methods and parameters to better test the code. In my opinion code should be agnostic of tests, since every additional "moving part" complicates the code, and more complex code means more chances for bugs. Also- the code does not need these methods (the tests do), so YAGNI applies. Having those methods like setting the faces of dices and hands in the tests on the other hand make perfect sense- they should be convenience methods of the tests, in my view.
Hi Dirk, glad you like the videos. Regarding the test helper methods: good point and I fully agree with you. Those methods are indeed better placed together with the tests to not 'contaminate' the original code.
@@ArjanCodes Arjan, would it not make sense to make these methods you use for testing somehow private? Maybe with "_" or "__"? Or... maybe you could dynamically attach a method of convenience right in the testing code? How would you do it?
Very cool refactoring!
Couple of things that made me double back:
- Die.set_face() can set values outside of Die.sides
- 41:22-41:32 the error in SameValueRule.points() is still shown, your demo-run used some other codebase?
You used enumerate() in some places without using the idx to index into your list - instead simply printing idx + 1 - could be simplified to enumerate(self.hand, 1) and remove the idx+1
Thanks - good points! I had to cut a few parts of the video, otherwise it would have become even longer, so there are a few places where an error is still shown. I did put the complete refactored code in GitHub which contains all the changes.
Great video!
Thanks Zack!
Your videos are so great. Always straight to the point and so well presented with a calm and pleasant voice. Btw, your clear pronunciation makes it very pleasant to listen to, even for non-native speakers.
Thank you Dolmet, happy you are enjoying the videos.
Arjan, I reeeeally like your videos! It has helped me structure my code so much more clearly! It is making a huge difference in my python learning journey!
I would really appreciate it if you could roast one of my projects hahahaha keep the amazing work!!
Very nice video, can you make please some video about Clean Architecture of Uncle Bob. It will be great to learn more about it.
Thank you Roman, good suggestion!
I very much appreciate the idea behind this video and the hard work that went into it. Me personally, I would not mind watching an even longer video just to your actual process of refactoring, not only the end result. Maybe let it be a multi part serious.
For the get/set functions at 10:00, why not use the @property decorator?
Yes this is the Pythonic way
Thanks for the efforts.
You’re welcome, Danish Hasan!
Great.
31:32 and prior where you define the rules for Aces, etc - you are assuming the dice are 6-sided and you still have duplicated code for Aces, Twos, Threes, etc. Should use a data structure to map 1:’Aces’, etc maybe look for a number to english package and use that but change “One” to Ace”. Other than that - VERY NICE!!
TODO: everything. :D
Holy hell, this is the most useful refactoring tutorial/coding tips/good practices video I've ever seen. Incredible work. I will continue to watch these in full for as long as you make them. Please continue making them!
You could even refine the combination tests by using combinations from itertools and counting all not repeating/permuting pairs (only once). 2 would be two_pairs, 3 three_of_a_kind, 4 full_house, 6 four_of_a_kind, and 10 would be the Yahtzee.
Of course the Fibonyahtzee cannot be tested this way. I think, it should be worth only 89 points (or 144).
Great video, all your content is amazing! The ammount of effort you put into creating the examples, shooting and editing is astounding. Tho the "thats what she said" killed me lol.
@9:00 Adding code to make testing easier is an AWFUL idea. You're concerned with code smells? That reeks like a rotten corpse!
1. You're forcing test-only code into production.
2. You're adding functionality not indicated in the project requirements.
3. To have 100% test coverage, you have to have a test that tests your test code.
4. You're not testing production functionality. You're testing that your code works if you use an undocumented feature.
5. You're adding potential security risks.
And for what? To make testing convenient? Prettier? It's testing. Who cares? Convenient? It's a cut and paste. If you want helper methods, put them in your test suite and don't make them visible to production code. Need to test times, or random values? Pass generators in production then mock them in test.
Don't do this. Don't allow this. Make sure this fails a code review.
Excellent Tutorial on Python Code Refactoring ! Thanks.
One of the few times where I get a reminder to press like and I'm like "yeah, I definitely need to do that", considering I have spent hours going through the original already so I could be ready for your notes and refactoring strategies. Great video. Are you still accepting submissions for refactoring videos?
I quite enjoyed the video. My nitpick would be is that the refactored code is not very pythonic. I mean, it does not take advantage of the data model (dunder medhods) and the standard library. I find that clases adhere to the data model (implement certain dunder methods to emulate the behaviour of some of the built in data structures) are easier to use.
For example, if we think that a Die is an integer (with range restrictions and a rolle method) and that a Hand a list of Die, then we end up with sum(hand) instead of hand.sum() and die == 5 instead of die.get_face() == 5, which I find easier to read.
I suspect that could be achieved by reaming Die.get_face to Die.__int__ and Hand.sum to Hand.__sum__ and maybe implementing Die.__eq__
@ArjanCodes, would you mind a PR with more details?
Thanks! When I posted this video, I was not yet fully committed to Python on my channel, as I am now. Since then I've started adhering to a more Pythonic standard for the code examples. I agree that here it would be nice to rely more on dunder methods to make the classes easier to use. I'll make sure to include that in follow-up examples/roasts.
@@ArjanCodes maybe you can roast your own code? I'm looking forward to the next in the series.
BTW, had quite some funn today refactoring your advanced version. So far have only done the hand and rules module. There is a bug on the small straight rule, the 1st clause (len==4) is unreachable... 100% coverage might not not the end goal, but +90% turns out to be quite useful.
I like your videos very much!
I have a small problem with your implementation of the scoreclass. Just checking if a frow has value 0 can lead to the fact, that I can write a row more than one time. I think there should be something like is_row_writable, so you can set a value just once.
I just wonder why we don't learn programming in the right (or more desirable) way from the very beginning? I made many tutorials here on RUclips and many of the programmers didn't use SOLID principles. No-one mentioned the clean code and TDD concepts. I have the impression that specially in embedded applications using Raspberry Pi, Esp-32 , Raspberry Pi Pico, Arduino, PIC or Stm32 the code isn't structured in that very clean and understandable way. Even official libraries used in for the mentioned hardware. I repeat: I just can't understand why is that so. I have heard that even in some university courses there are professors who seem not to follow those ideas and pass them on. Why???
I don't understand why one would need extremely simple one-line functions as in 24:13 where you defined rule_count which simply returns the number of rules. Why wouldn't the coder just write len(self.rules) instead of using this function?
I would like 3.165 sides on my dice computers can do anything these days!!
I have no fucking idea of most of the things you are doing or talking about, but I think I'm learning much more in this 45 min video than in 10 hours of python tutorials.
Thank you for the time spent!
Great quality and very useful content all over the channel
Thank you so much, Luc. I’m happy the content is helpful for you.
Great video.. thank you..
One remark.. what did you mean by "that's whats she said" at 29:15
Dude slow down and explain the starting code and what it does.
It's pretty unclear what the hell yahtzee is and the code.
I'm really enjoying your content and learning a lot👍 Quick question. You check for an empty string using len, as I do. But people always tell me to test as a Boolean. I think len is more readable and distinguishes None. What's your thinking?
Checking if an object is falsy/truthy by just writing "if obj:..." is faster (especially for the builtin types) and, as some say, more pythonic. There is a vid on YT (but can't remember right now the title) where a Python developer talks about it and presents statistics.
Do more code roast please
44:15 if you replace the old unittest with pytest, you can use real assert statements instead of all of those other calls
25:30 is it ok to use a plain `Exception`? As `except Exception:` is severely frowned upon aren't you raising an exception which can only crash the program?
These videos are really inspiring me to finally get around to refactoring my Discord bot implementing a multiplayer game with secret information. I got it to a point of basically functional last year, but the design is rife with the sort of code smell you're tackling in these vids, and I've found making adjustments to the original code very cumbersome. I particularly enjoyed this real-life example, giving me a wider variety of ideas and new perspectives.
If I were to make any requests, it would be (1) logging best practices and (2) how you test your code if it relies on data models from an external API.
In my example, I use discord.py's User model for getting names and sending PMs, as well as the Guild model for sending public messages. Making dummy instances of these classes to test offline got so difficult/unreliable that I've really only tested by manually putting it together on Discord as if I were playing the game with friends. And since it's a multiplayer game (min 5 players, max 10), I coded a bunch of "echobots" that respond to different command prefixes to simulate my extra players. Imagine me sitting here sending "0-echo !approve; 1-echo !reject; 2-echo !approve..." Very messy solution that has bogged down my development process quite a bit.
I think you skipped over some details with the explanation of computing the consecutive sum: the shown formula is n*(n-1)/2, but the implementation is (max+min)*(max-min-1)/2, i.e. max+min = max-min which is true only for min=0.
Correct. I had to skip some parts to keep the video length in check (which I ultimately failed in anyway, haha).
Very nice. Thank you for your efforts!
Such a useful video and thank you for spending your time going through everything in detail! There's definitely not enough of these videos around, so this is great!
Thanks so much Mike, glad you liked it!
while the refactoring thought process was quite enjoyable to watch, there are a couple issues with the final code with respect to the actual rules of the game, i think. the N of a kind for 1..6 value is *just* the sum of the faces of 1..6, not the whole hand.
the small straight should be the length of the set(sorted(l)) as a start... then have to check the len=4 for missing internal # in some fashion, and the sum test for == 10 (1..4) or 18 (3..6) would indicate a "bottom" straight or a "top" straight". sadly, the result for an 'inner" striaight, "14", is aliased, so need to check for lowest ==2 and highest ==5.
there is the concept of lots of good stuff for *multiple yahtzees*, which there is no hint of in the original code (you gotta work with what you got, i guess :D)
there are a couple bonuses that accrue at the end of the game, the "top half" of the sheet gives 35 for a sum of >= 63 in all the top categories (achieved by banking 3 of each, e.g.) and the "bottom half" has multiple yahtzee extra points.
might want to reconsider the commercial aspiration of this! ;)
Wow! That was a doozy. Now I need a beer and its only 11am. But hey, its Friday so what the hell.
Just imagine you’re in another timezone 😎.
are we related, you look like my uncles
Ah..... Saviour.... Please continue the series @ArjanCodes
Working on the next installment ;)
One thing I'm not so comfortable with this is how long you spent with broken code. I would have preferred to do this with several small intermediate refactorings and always make sure the code still works as expected.
Normally this is not how I work. I agree that small refactorings are better, unfortunately that doesn’t work in a video like this as it would become way too long (and this video already was extremely long).
Truly excellent video on separation of concerns and refactoring Arjan :) - A video suggestion I would love to see is the analysis on how you arrived at this structure for your classes and general architecture. Do you use UML or do you analyse the requirements and create classes based off of the SOLID principle. Any help with how to start a project or refactor existing ones with this in mind would be invaluable!
"If face is not None". Can you explain why not use "if face:" instead?
Thank you :)
You can definitely use that instead. I like to be explicit in conditional expressions and not rely on truthiness too much as it might not be clear in what cases you expect the condition to hold, leading to potential bugs.
almost whole code changed
What keyboard do you use?
can I submit a shell that I made in python ? it does a lot of stuff but since I made it before learning about software design, it is really badly implemented
Certainly! If you join my Discord server (discord.arjancodes.com) there’s a channel where you can submit your code.
lmao that's what she said
Not gonna lie, this was a pleasant video to watch. It’s not often that you see how other people conduct code reviews and that is valuable. Subscribed ❤️
Thank you for refactoring master class , looking forward for more videos from this series
This refactoring series is amazing. I think is good both for you and your audience
Thank you! This is awesome content. Keep making more of these!
Thanks and will do! :)
Do me! Do me! Hey Arjan, where can I send you my code so you can roast me? 🙏😁
Hi Angelo, you can submit your code roast request via my Discord channel. You can join using this link: discord.arjancodes.com.
Your videos are just too good. Thanks for your effort.
Thank you so much, I happy you like the videos!
Is this the only video from this series? It's so satisfying to watch
At the moment, yes. But there is more in the pipeline!
I've been a web developer for the last eight years and your videos have some of the best clear advice for application architecture I've come across. The relaxed atmosphere and high-quality of the recordings make a huge difference as well! You really know your stuff.
I'm watching your videos because I'm intending to switch from PHP web development to Python web development. I am really trying to internalize the Pythonic approach to coding, as opposed to just coding PHP-style in Python. These videos are helping me a lot, especially presented in the context of application design which is familiar to me from PHP MVCs.
I know this video is a year old already, but something in here has got me thinking on a code style topic that comes up for me a lot working with other developers in production contexts. That's the use of abbreviations in variable names. In this video you use the abbreviation `strs` for a List of strings in the `create_points_overview` method. Then you introduce a "typo"-category bug where you are referencing singular `str` instead of plural `strs` and you don't catch it until later. Thanks for including these kinds of bugs in your video and not editing them out. Bugs of this kind are a regular part of coding. I think developers of all experience levels encounter these "typo" kind of bugs and being aware of just how common they are is a great step towards having a coherent approach towards dealing with them..
Robert C. Martin makes what I think is a strong argument against the use of abbreviations for variable names in his Clean Code book. In my subjective point-of-view, without abbreviating the word `strings`, it would have been clearer that the variable name needed an `s` on the end. Even better, if the variable were to have a descriptive name like `point_rows`, then reading the code could become more conversational and it's easier to catch typos, especially of the plural/singular variety. My argument, lifted from Martin, goes like this: a descriptive approach to variable naming that aims for high specificity and doesn't use abbreviation can reduce the frequency of uncaught "typo"-category bugs in your code. Making code more comprehensible has additional value besides as well. This is all subjective, but I haven't heard a great argument *against* this approach.
Variable name abbreviation was ingrained in the learning docs for major languages like Java, so it's not really any wonder that is so common. Martin's non-abbreviated variable naming approach based on my subjective experience feels more common in the PHP world than it does in the Python world. Clean Code also suggests eliminating almost all code comments, which I am influenced by and I have reduced my code comments, though I still use code comments frequently. Any thoughts on the influence (or lack thereof) of the Clean Code approach in the Python community, of the use of abbreviations in variable naming, and variable naming in general? Might make a good video topic. Thanks for the excellent content!
Hi Stephen, thanks so much for sharing this! I really appreciate your message, and I'm glad that my content is valued so highly. Your suggestion is noted! ❤
Your videos are so valuable. Thanks.
Thank you Jim, glad you like them!
I'm kinda confused on your stance on "If you ain't gonna use it, don't implement it". You still separated view (print) from logic because we might publish it for some other device (iOS/Android/Web), but we know that we ain't gonna do it, so why refactor it for that possibility?
I think it helps to make a distinction between the overall design of a software application and specific features that you're adding to it.
For specific features, like adding a JSON export format to store the state of the game, only do that if you really need it, because building and testing features takes time that you can potentially invest elsewhere.
For the overall software design, it's crucial to design with separation in mind, because a) extending the system at a later stage is easier and b) you want to be able to test your software easily. In that sense, "if you ain't gonna use it" doesn't apply here. Having a more generic design is something that you actually WILL use, because it will save you time when you write your tests as well as when maintaining the software.
@@ArjanCodes got ya. Good point.
He means don't implement methods and features you don't use.
The refactoring here addresses design flaws.. you should design the code for scalability, portability, reusability and testing.
There are a lot of things to consider when writing code. Because generally speaking software changes all the time.. new requirements come and you need to design your code for change..
I love your code roasts , I learn alot from them
Thank you, Joshua! I'm going to post a new code roast video series soon.
This content is beyond gold. And this "refactoring code" seres is an awesome idea!! Here home we love your work and appreciate the work you put into it.
Glad you enjoy it!
In hand.py, line 16 why not use the any() function instead of liste comprehension? This way the if statement would be triggered on the first violating value instead of computing the entire list, then checking it's length. Also in the constructor why not use a list comprehension to initialise self.dice?
Loving your work :)
Hi, thanks and good suggestions! I did a lot of refactoring in this video (a bit too much for one video I realized later), which led to less optimal solutions in some cases. If I do a code roast again, I’ll probably do a shorter one and spend more time on finding the best way of rewriting the code.
This is easily one of the best programming channels out there. Your explanations are always very clear and easy to follow. As someone who just programs as a hobby and has no formal training, my codes used to be a complete mess, everything in one big smelly file and it was a nightmare to debug and change something. Just trying to implement better structure and some of your practises has improved my quality of life as an amateur coder a lot.
Thank you so much - glad you like the content!
My God, man, i love you. Thank you so much.
These code roast videos are SO GOOD
Thanks Felipe, glad you liked it!
@ArjanCodes It's a very cool concept! I have a question, I have noticed that you are using methods when properties could have done the same thing, is there a reason why you didn't use properties in these cases?
Thanks Kevin, there's no particular reason for using methods actually. Properties are a great option, but I forgot they existed in the heat of the moment ;).
Thank you Arjan, this is just great to better understand how the principles you talk about are applied in real life!
Thanks, glad you liked it!
thank you you're awesome
Thanks so much Fredieee, glad you liked the video!
Another one of your videos to go on the watch & rewatch to get more out of. Thanks for helping me improve so much.
You are so welcome!
Can you please make python logging videos for logging a project .
Thank you for the suggestion!
Nice I can tell this series will be popular. Vids are a bit long though.
Thanks Dave! Yes, this was quite long. I might split the next episode into parts to make it more easily digestible.
More of this!!
Coming really soon! ;)
Long but really good tips, thanks!
Glad it was helpful! And indeed quite long - if I do another code roast, I'll probably make it a bit more concise.
I love these code roast videos. Keep ‘em coming.
Thanks Henrik ;)
Pew die.py