9:27 IMO Copilot generated a bad test here. If the quality is 0, it's not going to drop any further, regardless of whether the item is Sulfuras or not. It would've made more sense to test this with any other value. That's why you gotta check your AI generated code!
I agree. A few of these tests have confounding factors and so they aren't testing the specific functionality they say they are testing. Otherwise, I'm genuinely surprised how copilot is able to create these from the rest of the files/requirements
I'm in the process of refactoring a pile of legacy code right now which looks a lot like the example you started with, so this video really helped me frame my approach more clearly. One thing I would add is that you may need to go through this whole process (steps 1-5) several times before you're happy with the result, as your understanding of the system gradually improves with each successive refactor and you think of more test cases to add.
The BEST video on Refactoring, I could create a refactoring checklist for myself, and it shows perfectly why unit testing is inportant. Thank you very much!!
I am a PhD student and my suprvisor told me at the beginning of my studies to develop a hydrological model as a part of my thesis and to create a desktop software with GUI for it. I have never touched programming before but your videos about everything made my learning so much easier and I really appreciate the refactoring advices you have shown not just in this video. Also tutorials on dataclasses, protocols and other stuff really helped me with finishing this rather large project of mine. So thank you for making my studies so much easier! Maybe I should provide this project for smelly code roasting videos.
I've been using knowledge from your videos and I noticed I am writing smaller classes, use functions more, and use composition more. I am still learning all the design patterns, might have to read a book now. Regardless, my code has improved as a result. It is less bloated, writing documentation instead of comments, more reusable. I'll have to hear from others if what I write is readable though lol. Thanks a lot!
I'm always amazed about how you tackle problems by seeing it through step-by-step in a structured manner and with such ease. Really somethin I want to adopt and get better at for myself!
Please more of these videos!!! They are great to learn from an experienced programmer point of view. Keep the good work! Edit: After rewatching the video, I have a question. What's the purpose of the ItemUpdater protocol class you define?
ItemUpdater defined a Protocol that’s later used in his method definitions. It basically defines the common methods that any item updater would offer, such that the method definition (or any other static type declaration) doesn’t have to be specific to the class given. In this way, it’s similar to Abstract Base Classes (ABC), with a little more flexibility since the “subclasses” need not inherit explicitly from the Protocol (“parent”) class. You can search about duck typing (that’s not a typo lol) and python Protocols to learn more about this.
@@abdallahel-falou3849 Hi, thanks for your reply! I already knew what a protocol was... still appreciate it! I looked at the repo and noticed that it uses the ItemUpdater class in the ITEM_UPDATERS dictionary. That was the missing piece of the puzzle!
@@abdallahel-falou3849 DefaultItemUpdater and all the derived classes are indeed compatible with the ItemUpdater protocol but, as far as I can see, ItemUpdater is not mentioned as a type hint anywhere. item_updater is simply retrieved from a dictionary. You could theoretically put an incompatible class into that dictionary and that would break update_quality_single() method. If we removed ItemUpdater protocol class, the code would still work, right?
"Have you ever been in a situation where you had to work on legacy code?" Working on legacy code is half my job. The other half is writing new legacy code.
This is got to be one of your best videos yet! I just want to comment on my recent experience with this. In real life refactoring we may not have such clear starting instructions. So step one of analysing the code will often include writing down that behaviour. I usually find that simple flowchart Mermaid diagrams during the first step help figuring out what the code does so we can write those tests 👍 GREAT video ❤
I'm taking away two things from this video: a) there are actually refactoring katas b) you are very good at driving home the message of test first. You may read this as TDD, but whatever the case, the existing system is performing some task according to current requirements, so you have to make sure you don't break that. Many thanks Arjan!
Your opening remark about jumping out of a window made me laugh out loud. It reminded me about my job as a software developer just before I retired several years ago, where my team was faced with converting a mountain of the most convoluted and awful-looking legacy code to a new technology. How that legacy code came into being was due mainly to a lack of proper software-engineering management, e.g. how to assure quality code, how to assure developers are adequately trained in how to write quality code, and providing the necessary oversight of unskilled developers. Most of that code was churned out by several very smart unskilled developers.
We recently practiced this kata at work in our TDD Dojo using approval testing and code coverage reporting. Felt like black magic sometimes, but it was very effective.
Your never more than 50 test was interesting, as in not sure it tested for the scenario. I really appreciated seeing how someone else writes unit test cases. Great video.
Great stuff. As you were starting and I could see some of the original, I was like: "Oh, we should refactor this... split sellin and quality..." My mind started racing with ideas lol. Fun to see the design you came up with. One thing with older languages/ projects and legacy. More than once I came across a difference between the 'requirements' and the 'performance' of the legacy code. For one reason or another, the code didn't match the documentation. So when writing unit tests, if there is a conflict have to go back to customer to try and identify why. Often a lazy programmer in the past fixed / updated something but didn't update the documentation. :(
Thank you for this, my day job requires me to work with legacy code that was written in a hurry! Maybe I can slowly chip away at it using this framework
Have you got tips on how to make a PR when refactoring? Doing to step by step it makes sense, but if someone’s reviewing the final PR it can look like a sea of red and green
You definitely don’t want to do all of this in a single PR. At least each step could be a PR. For example, you could first do a PR that adds the unit tests so everyone involved agrees that the tests properly reflect what the software is supposed to do and whether this is enough tests to feel confident to change the code. Then, move on to step by step refactoring and make a planning before of how to organize that into separate PRs. Then a final PR could be about adding the new feature. But it also depends on how large the codebase is, so you might want to split things up even more.
I was looking through my external hdd the other day and found code I wrote like over a decade ago. As a self-taught programmer, resources back in the day on how to write good and efficient code were not even close to being as abundant as today. I thought it would be fun to sort of improve on the code, but of course it was a mess. There were no tests, classes with hundreds of lines of code and everything in one giant file. Not even halfway through refractoring I figured it would be easier to just completely rewrite it from scratch 😅
For these type of work I recommend everybody to read Martin Fowler's amazing book, Working effectively with legacy code. What Arjan does at the beginning called approval testing, basically cementing down the current behavior with tests. From there you can refactor and write more unit tests. The goal with legacy code is to refactor enough to isolate your attachment point where you can add your new functionality. The new functionality should be well tested and written following TDD rules.
Very helpful video! This seems like an ideal place to use a class hierarchy, where the cheeses know how to age themselves, etc. This would encapsulate the knowledge nicely. Perhaps this wasn’t appropriate for a refactoring video?
Great video! About your 3rd step, create a safety net, how would you approach that if the legacy code is difficult to unit test in it's current state? For example, if the legacy code has hard dependencies to hardware and servers which is not currently available, but functions/classes tries to access those entities as soon as they are executed or created.
Great video! But the example shown is usually not the problem I run into. The example code was very straight-on, declaratively written, whereas the legacy code I usually run into was written by someone who read half of Clean Code and then decided he knew everything. I.e., it's chock-full of unnecessary abstractions and indirection and relies too much on inheritance. Do you have any tips on dealing with this type of legacy code?
As always, amazing content! Thank you for producing these videos, I'm confident to say that they help a lot of developers out there. I just wanted to point out that I always had a problem with these updater objects like the one you implemented. In my opinion, creating such objects often results in coupling between a dataclass (like Item in your example) and an updator class (the DefaultUpdator here). Wouldn't it be more logical to create an implementation where the behavior is directly connected with the state of the class, for example an AgedBrie class that implements a update_sell_in and an update_quality method?
Thank you! Unfortunately, we can’t change the Item class because the requirements don’t allow it. So to do what you propose, AgedBrie would need to be a subclass of Item. This introduces the problem that you can now have items with a name that’s different from its type (I.e. you can create an AgedBrie instance named “Conjured”). To avoid confusion, I decided to use updater objects instead. It’s not what I would do if I created the code from scratch, but it’s a compromise and a good illustration of the things you sometimes need to do in the real world.
Great video! I think this kind of stuff is really missing from "normal" education. It's hard to put it in a book or an article, because refactoring is dynamic, but video works great. Are you familiar with the Mikado method? It's quite similar to what you described here, but it is designed to also handle bigger changes, which need more steps than a single refactoring. The "The Mikado Method" book is quite good. There are also a few articles and videos if you search a bit. I highly recommend it.
Since Item is alrady an object, wouldn't make more sense to define the methods inside the class (and subclasses) so that the object is responsible for it's own updates?
@@ArjanCodes thank you very much, I didn't catch that. May I ask a "weird" question? Have you got any plan to show some use cases of BDD using Behave in python? It's more a large-team feature but it might also be useful for solo programmers or small teams.
One problem I have with writing tests for legacy code before refactoring is, unlike a kata, we rarely have any specifications and so have to dig through the mess to try to workout what it's supposed to be doing anyway. (laughs)
I feel myself being pulled towards a rabbit hole on this example, where I want to build this exercise from scratch. Instead of a numeric quality value, a more real world example would be a purchase date. Then you only have to care whether the depreciation is linear or exponential. Inflation vs deflation is simply a matter of a positive or negative value being passed as the rate. In this way all items could be boiled down to two methods, yeah?
Hi, is there any chances you could do localized pricing for your courses? I just checked the price for the mindset one and it costs the minimum wage in brazil. I look forward to learning more with your videos and hopefully, courses.
Thanks for the great videos, Arjan, big fan. AFAIK the solution presented here doesn't fulfill the requirements. The requirements doc says there will be multiple conjured items, and this would need an exhaustive list of all conjured items in advance to function. Instead, it would be better for each ItemUpdater to have an acceptance method that checks if it applies to an item, so that you could match all the items beginning with "Conjured".
I've often started refactoring and then run into bugs that turned out to be present in the original code base. Lesson learned: Run your unit tests BEFORE starting refactoring.
Around Minute 20 you change the if statements. I wondered if that may slow down the code if the original programmer tested for the more common case, hence avoiding having to call the else statement too often?
This is similar to a project in which I'm working right now. Only each file is 1000+ lines long and for a complete suite of 20+ files per module....a nightmare, without any obvious test cases....I'm kind of pulling out my hair, but I am now about 70% done....nightmare!
16:25 "Don't have to worry about the constraints that it must be between 0 and 50, because it is built into these functions" False, it is possible to provide increase_item_quality with a negative value, allowing the quality to become negative and violate the constraint. This is also true for the decrease_item_quality, but vice-versa.
@@ArjanCodes Yup! What about the bounds then? Do you recommend that the bounds' default values should continue to be set in the function? What about introducing attributes to the ItemUpdater for representing those bounds? (e.g. qualityUpperBound) That would allow different Items to have different maximum Quality - but might require a Template pattern. ... I can't see those attributes being added to Item, as it would allow Items with the same name to have different maximum Qualities. So it makes sense to add them to the ItemUpdater, and allow sub classes to override them (when need be).
Adding the bounds to the item updaters would be a really simple change to the code that would allow different values for different item types. Adding them to the Item class would allow for even more flexibility since you can then change them per item. However, none of that is specified in the requirements, so I wouldn’t spend time on it unless it becomes a requirement 😁.
@@ArjanCodes Fair enough :D I haven't used Python in quite a while, so I don't watch as often as I used to. But wanted to say: Thank you for the video, I enjoyed it as always! ^_^
I liked the video, and I found something that is not its main topic, but interesting for me. This solution doesn't satisfy all the SOLID principles. According to Liskov Substitution Principle, you shouldn't have replaced the default behavior with an other behavior. (And if I'm right it is also Open-Close Principle violation.) Maybe I wouldn't make the DefaultItemUpdater as the parent of the others. The DefaultItemUpdater would be one implementation of the protocol, and all of the others would implement both methods, not using the default methods. But the question is not this. Is my version makes anything more flexible? Yours is a kind of Don't Repeat Yourself version, and DRY sometimes contradicts SOLID as I see. It seems to me that whether to use the DRY or the pure SOLID version is a trade-off. Less code vs more flexibility.
I spent 5 months trying to refactor a legacy codebase that was running in production for 3 years with no1 across it... I couldn't figure out TTD cause it had about 8 x 400 lines of code functions with database connections and pushing changes to systems etc (I've now learnt how to mock those variables but didn't at the time)... 5 months later I finally had data and was able to test it... couldn't get it going (broke something). Scrapped the whole thing and rewrote it in 500 lines of code over a weekend (and it out performs the old model). Don't skimp on tests :P but better yet - don't skimp on 'is it useful' step :P I still have no idea how that other code works :/
33:50 Inverting logic - What about all the lessons on testing False is faster than testing true? Do your tests and refactoring in these areas not also require timing / performance testing?
I wouldn’t prioritize performance if your goal is to make the code easier to maintain. In many cases performance optimizations add complexity as opposed to making things simpler. That’s why you typically do them once you’re happy with the overall design of the code. On top of that, I don’t expect the performance difference between True or False comparison to have any impact whatsoever on this particular use case. If performance really becomes a bottleneck, you’d probably don’t want to use Python anyway.
Is there a reason you chose to create updater classes rather than going for inheritance on the item and adding the update quality and sell in methods directly to the Item subclasses? Or was that just a rule for the Kata?
The issue with inheriting from Item and adding the update methods is that to do it ‘properly’, you would need to first create a subclass with those two methods, and then create subclasses of that subclass to provide the implementations for each of the item types. On top of that, it introduces the potential for inconsistent objects as it’s now possible to have an object that carries the name of one type, but it’s actually an implementation of another type via the subclass. With a separate ItemUpdater hierarchy, you avoid these problems, though of course you can still choose to use the wrong ItemUpdater on an item, at least it’s not encoded in the object itself.
Good summary! What you describe as "invert logic" for step 4 (Refactoring Step-by-Step) could be fleshed out a bit by mentioning the SOLID principles. Not that you would have to clean up the entire codebase to comply 100% with all SOLID principles. But using them as a checklist for the code that you intend to change anyway would be helpful, I find.
I would have created each item (Sulfuras, Aged Brie, etc.) as a subclass of Item, and I would have added the default behavior of update_quality and update_sell_in as methods of the base class Item. Then, for each specific method, I would have overriden the base methods when defining each specific subclass. Alternatively, I would have added an instance attribute called updater, that contains the specific updater that the item needs. This way, I don't need to have a dictionary to set each specific updater, rather they are assigned when each item is created (I only need to add the updater in each subclass definition, inside the __init__ block). And a third alternative would be to use the dictionary but instead of storing the result in a local variable item_updater, to store it in an attribute of the Item object, for example the updater attribute. To me, this way it makes it clear that every item has an updater, either the default or a specific one.
@@Decrupt Right, but you could still inherit from the Item class and make these changes in the subclass. Maybe in a subclass called ItemCustom to make it clear that we are customizing the original Item class for the purposes of the requirements. In that sense, we could also call it ItemCustomForRequirements to make the intent even more explicit. But at that point, I think it will depend on the specific circumstances (any keywords/terminology that we use in the project to refer to specific requirements or custom versions of imported classes/files, what conventions are established, what makes it more readable for the team as a whole, etc.).
The reason that I didn’t create subclasses of Item is that it introduces the problem that you can then have items with a name that’s different from its type (I.e. you can create an AgedBrie instance named “Conjured”). To avoid confusion, I decided to use updater objects instead. It’s not what I would do if I created the code from scratch and was allowed to change Item, but it’s a compromise and a good illustration of the things you sometimes need to do in the real world.
Yeah well done. But when refactoring an actual legacy codebase, you need to make the code testable first. And that's the tricky part because there's no safety net. So better get comfortable to refactor without tons of tests 😂
Great video once again, Arjan! Reminded me of a very good talk Kevin Henney once gave about the Gilded Rose Kata and refactoring, he called it "Refactoring Driven Development" :) th-cam.com/video/kTcDBYCpj7Q/w-d-xo.html
👷 Join the FREE Code Diagnosis Workshop to help you review code more effectively using my 3-Factor Diagnosis Framework: www.arjancodes.com/diagnosis
This is surreal, never have I ever seen such a clear requirements specification!
Clearly not a real requirements spec. No User is ever that detailed or knowledgeable about the changes they want.
My first thought.
This is literally like one of those satisfying pressure cleaner videos just for python code. Thanks Arjan!
Hahaha, that's the best comparison I've seen so far! :)
THIS!!! This is an amazing video. Make more of these, it explains exactly how to go from beginner to intermediate. It’s so clear!
Thank you so much- glad you liked it!
9:27 IMO Copilot generated a bad test here. If the quality is 0, it's not going to drop any further, regardless of whether the item is Sulfuras or not. It would've made more sense to test this with any other value. That's why you gotta check your AI generated code!
I agree. A few of these tests have confounding factors and so they aren't testing the specific functionality they say they are testing. Otherwise, I'm genuinely surprised how copilot is able to create these from the rest of the files/requirements
I'm in the process of refactoring a pile of legacy code right now which looks a lot like the example you started with, so this video really helped me frame my approach more clearly. One thing I would add is that you may need to go through this whole process (steps 1-5) several times before you're happy with the result, as your understanding of the system gradually improves with each successive refactor and you think of more test cases to add.
The BEST video on Refactoring,
I could create a refactoring checklist for myself,
and it shows perfectly why unit testing is inportant.
Thank you very much!!
Thank you - you’re welcome!
I am a PhD student and my suprvisor told me at the beginning of my studies to develop a hydrological model as a part of my thesis and to create a desktop software with GUI for it. I have never touched programming before but your videos about everything made my learning so much easier and I really appreciate the refactoring advices you have shown not just in this video. Also tutorials on dataclasses, protocols and other stuff really helped me with finishing this rather large project of mine. So thank you for making my studies so much easier! Maybe I should provide this project for smelly code roasting videos.
Wow! That's amazing! If you join my Discord channel, there's space for code roasts/smells, you can submit your project there. :)
I've been using knowledge from your videos and I noticed I am writing smaller classes, use functions more, and use composition more. I am still learning all the design patterns, might have to read a book now. Regardless, my code has improved as a result. It is less bloated, writing documentation instead of comments, more reusable. I'll have to hear from others if what I write is readable though lol. Thanks a lot!
Wow, that's actually amazing to read! Thanks so much for your kind words. :)
I'm always amazed about how you tackle problems by seeing it through step-by-step in a structured manner and with such ease.
Really somethin I want to adopt and get better at for myself!
Please more of these videos!!! They are great to learn from an experienced programmer point of view. Keep the good work!
Edit: After rewatching the video, I have a question. What's the purpose of the ItemUpdater protocol class you define?
ItemUpdater defined a Protocol that’s later used in his method definitions. It basically defines the common methods that any item updater would offer, such that the method definition (or any other static type declaration) doesn’t have to be specific to the class given.
In this way, it’s similar to Abstract Base Classes (ABC), with a little more flexibility since the “subclasses” need not inherit explicitly from the Protocol (“parent”) class.
You can search about duck typing (that’s not a typo lol) and python Protocols to learn more about this.
@@abdallahel-falou3849 Hi, thanks for your reply! I already knew what a protocol was... still appreciate it!
I looked at the repo and noticed that it uses the ItemUpdater class in the ITEM_UPDATERS dictionary. That was the missing piece of the puzzle!
@@abdallahel-falou3849 DefaultItemUpdater and all the derived classes are indeed compatible with the ItemUpdater protocol but, as far as I can see, ItemUpdater is not mentioned as a type hint anywhere. item_updater is simply retrieved from a dictionary. You could theoretically put an incompatible class into that dictionary and that would break update_quality_single() method.
If we removed ItemUpdater protocol class, the code would still work, right?
"Have you ever been in a situation where you had to work on legacy code?"
Working on legacy code is half my job. The other half is writing new legacy code.
Nice 😊
I'm also pretty sure that, at some point, every dev touches "legacy code". It also depends on your definition of legacy.
This is got to be one of your best videos yet!
I just want to comment on my recent experience with this. In real life refactoring we may not have such clear starting instructions. So step one of analysing the code will often include writing down that behaviour.
I usually find that simple flowchart Mermaid diagrams during the first step help figuring out what the code does so we can write those tests 👍
GREAT video ❤
Thank you so much!
This video was very useful, no matter what language you're working in.
Thank you Jeffrey! Glad to hear you found it helpful.
@@ArjanCodes This is also one of the best videos I've seen on TDD.
Nice use of min & max functions. Have never thought about that before. 😅
I've never thought of using max like that before!
I'm taking away two things from this video:
a) there are actually refactoring katas
b) you are very good at driving home the message of test first. You may read this as TDD, but whatever the case, the existing system is performing some task according to current requirements, so you have to make sure you don't break that.
Many thanks Arjan!
You're very welcome! :)
Your opening remark about jumping out of a window made me laugh out loud. It reminded me about my job as a software developer just before I retired several years ago, where my team was faced with converting a mountain of the most convoluted and awful-looking legacy code to a new technology. How that legacy code came into being was due mainly to a lack of proper software-engineering management, e.g. how to assure quality code, how to assure developers are adequately trained in how to write quality code, and providing the necessary oversight of unskilled developers. Most of that code was churned out by several very smart unskilled developers.
We recently practiced this kata at work in our TDD Dojo using approval testing and code coverage reporting. Felt like black magic sometimes, but it was very effective.
I've looked at someone else's brand new code and wanted to jump out a window.
This
LOL Haven't we all.
I've looked at my old code and wanted to jump out of a window
@@marcolaube2957 I thought that was such a universal experience that I didn't bother mentioning it. XD
It’s become clear to me that we immediately need to stop putting developers in offices with windows.
Can't reccomend your content enough to colleagues. Thanks again Arjan! Always enjoy the refactor type videos
Thank you so much! I really appreciate it. :)
Thank you very much for this video, because I just saw this when I had to refactor old code from the company
Glad I could help!
Your never more than 50 test was interesting, as in not sure it tested for the scenario.
I really appreciated seeing how someone else writes unit test cases. Great video.
Glad you enjoyed it!
Great stuff. As you were starting and I could see some of the original, I was like: "Oh, we should refactor this... split sellin and quality..." My mind started racing with ideas lol. Fun to see the design you came up with.
One thing with older languages/ projects and legacy. More than once I came across a difference between the 'requirements' and the 'performance' of the legacy code. For one reason or another, the code didn't match the documentation. So when writing unit tests, if there is a conflict have to go back to customer to try and identify why. Often a lazy programmer in the past fixed / updated something but didn't update the documentation. :(
Seeing your thinking process here is so valuable. Thank you very much!
One of the most helpful and instructive videos about programming I've ever seen!! Thanks, Arjan.
Wow, thanks!
My daily dose of learning. Thank you Arjan.
Weekly*
You’re welcome! 😊
Thank you for this, my day job requires me to work with legacy code that was written in a hurry! Maybe I can slowly chip away at it using this framework
Nice content. One can learn so much with an example like this one. Thank you!
Thank you for the kind words, Gustavo! :)
Have you got tips on how to make a PR when refactoring? Doing to step by step it makes sense, but if someone’s reviewing the final PR it can look like a sea of red and green
You definitely don’t want to do all of this in a single PR. At least each step could be a PR. For example, you could first do a PR that adds the unit tests so everyone involved agrees that the tests properly reflect what the software is supposed to do and whether this is enough tests to feel confident to change the code. Then, move on to step by step refactoring and make a planning before of how to organize that into separate PRs. Then a final PR could be about adding the new feature. But it also depends on how large the codebase is, so you might want to split things up even more.
I wish my legacy code at work looked like this. Or had these clear requirements lol.
I was looking through my external hdd the other day and found code I wrote like over a decade ago. As a self-taught programmer, resources back in the day on how to write good and efficient code were not even close to being as abundant as today. I thought it would be fun to sort of improve on the code, but of course it was a mess. There were no tests, classes with hundreds of lines of code and everything in one giant file. Not even halfway through refractoring I figured it would be easier to just completely rewrite it from scratch 😅
Great video! I think this is the best tutorial on the subject I watched
Wow, thanks! Glad you think that :)
Step #3 is the capstone! Thanks!
Loved the video, thank you, Arjan, for taking the viewer step by step out off this terrible code into something much much better.
Glad you enjoyed it!
For these type of work I recommend everybody to read Martin Fowler's amazing book, Working effectively with legacy code.
What Arjan does at the beginning called approval testing, basically cementing down the current behavior with tests. From there you can refactor and write more unit tests. The goal with legacy code is to refactor enough to isolate your attachment point where you can add your new functionality. The new functionality should be well tested and written following TDD rules.
Great suggestion!
Awesome video! Nice work Arjan!
Thanks! Glad you liked it
Great video. I consider all code in production legacy code.
Amazing content. Thanks Arjan
Great, thank you for your time and sharing knowledge!
My pleasure! Glad it was helpful! :)
Very helpful video!
This seems like an ideal place to use a class hierarchy, where the cheeses know how to age themselves, etc. This would encapsulate the knowledge nicely.
Perhaps this wasn’t appropriate for a refactoring video?
This is so great, do you think it is worth creating a video about Katas? Pros / cons + some go to resources ranked by level of difficulty or topic?
Great video! About your 3rd step, create a safety net, how would you approach that if the legacy code is difficult to unit test in it's current state?
For example, if the legacy code has hard dependencies to hardware and servers which is not currently available, but functions/classes tries to access those entities as soon as they are executed or created.
Great video! But the example shown is usually not the problem I run into. The example code was very straight-on, declaratively written, whereas the legacy code I usually run into was written by someone who read half of Clean Code and then decided he knew everything. I.e., it's chock-full of unnecessary abstractions and indirection and relies too much on inheritance. Do you have any tips on dealing with this type of legacy code?
If you have a codebase you’re willing to share, you could submit it as a code roast and then I might cover it on the channel.
As always, amazing content! Thank you for producing these videos, I'm confident to say that they help a lot of developers out there. I just wanted to point out that I always had a problem with these updater objects like the one you implemented. In my opinion, creating such objects often results in coupling between a dataclass (like Item in your example) and an updator class (the DefaultUpdator here). Wouldn't it be more logical to create an implementation where the behavior is directly connected with the state of the class, for example an AgedBrie class that implements a update_sell_in and an update_quality method?
Thank you! Unfortunately, we can’t change the Item class because the requirements don’t allow it. So to do what you propose, AgedBrie would need to be a subclass of Item. This introduces the problem that you can now have items with a name that’s different from its type (I.e. you can create an AgedBrie instance named “Conjured”). To avoid confusion, I decided to use updater objects instead. It’s not what I would do if I created the code from scratch, but it’s a compromise and a good illustration of the things you sometimes need to do in the real world.
Great video! I think this kind of stuff is really missing from "normal" education. It's hard to put it in a book or an article, because refactoring is dynamic, but video works great.
Are you familiar with the Mikado method? It's quite similar to what you described here, but it is designed to also handle bigger changes, which need more steps than a single refactoring. The "The Mikado Method" book is quite good. There are also a few articles and videos if you search a bit. I highly recommend it.
Since Item is alrady an object, wouldn't make more sense to define the methods inside the class (and subclasses) so that the object is responsible for it's own updates?
Yes, it would. However, the rule of the kata is that you’re not allowed to change the Item class.
@@ArjanCodes thank you very much, I didn't catch that. May I ask a "weird" question? Have you got any plan to show some use cases of BDD using Behave in python? It's more a large-team feature but it might also be useful for solo programmers or small teams.
Great Informative video, thanks for sharing!
Glad it was helpful!
I love the format.
Thanks!!
One problem I have with writing tests for legacy code before refactoring is, unlike a kata, we rarely have any specifications and so have to dig through the mess to try to workout what it's supposed to be doing anyway. (laughs)
I feel myself being pulled towards a rabbit hole on this example, where I want to build this exercise from scratch. Instead of a numeric quality value, a more real world example would be a purchase date. Then you only have to care whether the depreciation is linear or exponential. Inflation vs deflation is simply a matter of a positive or negative value being passed as the rate. In this way all items could be boiled down to two methods, yeah?
Very satisfying video to watch, thank you
Thanks, Luna! Happy you enjoyed it.
Keep making more of those :)
This is beautiful!
Hi, is there any chances you could do localized pricing for your courses? I just checked the price for the mindset one and it costs the minimum wage in brazil. I look forward to learning more with your videos and hopefully, courses.
Hi, can you email us at support@arjancodes.com? We might be able to help.
Wonderful video as usual. You have never used cyclomatic complexity to evaluate the issues of a code. Is there any reason?
Another great amd interesting video, very useful, thanks
Glad you enjoyed it! Thanks :)
Thanks for the great videos, Arjan, big fan. AFAIK the solution presented here doesn't fulfill the requirements. The requirements doc says there will be multiple conjured items, and this would need an exhaustive list of all conjured items in advance to function. Instead, it would be better for each ItemUpdater to have an acceptance method that checks if it applies to an item, so that you could match all the items beginning with "Conjured".
Why use a Protocol instead of an abstract class? Thx for the video!
Either one works. I like protocols because they fit well with Python’s duck typing system and they don’t require an inheritance relationship to work.
I've often started refactoring and then run into bugs that turned out to be present in the original code base. Lesson learned: Run your unit tests BEFORE starting refactoring.
👏Very nicely done!
Thank you!
Around Minute 20 you change the if statements. I wondered if that may slow down the code if the original programmer tested for the more common case, hence avoiding having to call the else statement too often?
This is similar to a project in which I'm working right now. Only each file is 1000+ lines long and for a complete suite of 20+ files per module....a nightmare, without any obvious test cases....I'm kind of pulling out my hair, but I am now about 70% done....nightmare!
You can do this! 💪🏻
16:25 "Don't have to worry about the constraints that it must be between 0 and 50, because it is built into these functions"
False, it is possible to provide increase_item_quality with a negative value, allowing the quality to become negative and violate the constraint.
This is also true for the decrease_item_quality, but vice-versa.
So technically speaking, the only thing that the max() and min() functions do ensure is that ONE of the constraints is not violated.
An easy solution would be to merge the two functions into a single one that combines a min+max.
@@ArjanCodes Yup!
What about the bounds then? Do you recommend that the bounds' default values should continue to be set in the function?
What about introducing attributes to the ItemUpdater for representing those bounds? (e.g. qualityUpperBound)
That would allow different Items to have different maximum Quality - but might require a Template pattern.
...
I can't see those attributes being added to Item, as it would allow Items with the same name to have different maximum Qualities.
So it makes sense to add them to the ItemUpdater, and allow sub classes to override them (when need be).
Adding the bounds to the item updaters would be a really simple change to the code that would allow different values for different item types. Adding them to the Item class would allow for even more flexibility since you can then change them per item. However, none of that is specified in the requirements, so I wouldn’t spend time on it unless it becomes a requirement 😁.
@@ArjanCodes Fair enough :D
I haven't used Python in quite a while, so I don't watch as often as I used to. But wanted to say:
Thank you for the video, I enjoyed it as always! ^_^
I liked the video, and I found something that is not its main topic, but interesting for me.
This solution doesn't satisfy all the SOLID principles. According to Liskov Substitution Principle, you shouldn't have replaced the default behavior with an other behavior. (And if I'm right it is also Open-Close Principle violation.) Maybe I wouldn't make the DefaultItemUpdater as the parent of the others. The DefaultItemUpdater would be one implementation of the protocol, and all of the others would implement both methods, not using the default methods.
But the question is not this. Is my version makes anything more flexible? Yours is a kind of Don't Repeat Yourself version, and DRY sometimes contradicts SOLID as I see. It seems to me that whether to use the DRY or the pure SOLID version is a trade-off. Less code vs more flexibility.
I spent 5 months trying to refactor a legacy codebase that was running in production for 3 years with no1 across it...
I couldn't figure out TTD cause it had about 8 x 400 lines of code functions with database connections and pushing changes to systems etc (I've now learnt how to mock those variables but didn't at the time)... 5 months later I finally had data and was able to test it... couldn't get it going (broke something).
Scrapped the whole thing and rewrote it in 500 lines of code over a weekend (and it out performs the old model).
Don't skimp on tests :P but better yet - don't skimp on 'is it useful' step :P
I still have no idea how that other code works :/
33:50 Inverting logic - What about all the lessons on testing False is faster than testing true? Do your tests and refactoring in these areas not also require timing / performance testing?
I wouldn’t prioritize performance if your goal is to make the code easier to maintain. In many cases performance optimizations add complexity as opposed to making things simpler. That’s why you typically do them once you’re happy with the overall design of the code. On top of that, I don’t expect the performance difference between True or False comparison to have any impact whatsoever on this particular use case. If performance really becomes a bottleneck, you’d probably don’t want to use Python anyway.
@@ArjanCodes Thanks for answer
I like how procedural spaghetti code got better and better until it eventually became overengineered OOP.
Is there a reason you chose to create updater classes rather than going for inheritance on the item and adding the update quality and sell in methods directly to the Item subclasses? Or was that just a rule for the Kata?
The issue with inheriting from Item and adding the update methods is that to do it ‘properly’, you would need to first create a subclass with those two methods, and then create subclasses of that subclass to provide the implementations for each of the item types. On top of that, it introduces the potential for inconsistent objects as it’s now possible to have an object that carries the name of one type, but it’s actually an implementation of another type via the subclass. With a separate ItemUpdater hierarchy, you avoid these problems, though of course you can still choose to use the wrong ItemUpdater on an item, at least it’s not encoded in the object itself.
How to effectively manage race conditions using python with redis?
I'm there now... And I wrote it 11 years ago...
im lost at your Item protocol, is it not used or inherited?
Good summary!
What you describe as "invert logic" for step 4 (Refactoring Step-by-Step) could be fleshed out a bit by mentioning the SOLID principles.
Not that you would have to clean up the entire codebase to comply 100% with all SOLID principles. But using them as a checklist for the code that you intend to change anyway would be helpful, I find.
❤
Safety Net 😆❣
I would have created each item (Sulfuras, Aged Brie, etc.) as a subclass of Item, and I would have added the default behavior of update_quality and update_sell_in as methods of the base class Item. Then, for each specific method, I would have overriden the base methods when defining each specific subclass. Alternatively, I would have added an instance attribute called updater, that contains the specific updater that the item needs. This way, I don't need to have a dictionary to set each specific updater, rather they are assigned when each item is created (I only need to add the updater in each subclass definition, inside the __init__ block). And a third alternative would be to use the dictionary but instead of storing the result in a local variable item_updater, to store it in an attribute of the Item object, for example the updater attribute. To me, this way it makes it clear that every item has an updater, either the default or a specific one.
Definitely agree, though in this refactoring you weren’t allowed to edit the item class/file.
@@Decrupt Right, but you could still inherit from the Item class and make these changes in the subclass. Maybe in a subclass called ItemCustom to make it clear that we are customizing the original Item class for the purposes of the requirements. In that sense, we could also call it ItemCustomForRequirements to make the intent even more explicit. But at that point, I think it will depend on the specific circumstances (any keywords/terminology that we use in the project to refer to specific requirements or custom versions of imported classes/files, what conventions are established, what makes it more readable for the team as a whole, etc.).
The reason that I didn’t create subclasses of Item is that it introduces the problem that you can then have items with a name that’s different from its type (I.e. you can create an AgedBrie instance named “Conjured”). To avoid confusion, I decided to use updater objects instead. It’s not what I would do if I created the code from scratch and was allowed to change Item, but it’s a compromise and a good illustration of the things you sometimes need to do in the real world.
the worst thing is working on legacy code that is also poorly writed, It makes it both unreadable, unmanteinable and makes work really feel bad
Kata as in "to perform a Kata in Karate"?
I am going through the refactoring legacy code hell right now. I am dealing with the software written by a really dumb engineer... me 10 years ago
Hahaha! Good luck :)
Yeah well done. But when refactoring an actual legacy codebase, you need to make the code testable first. And that's the tricky part because there's no safety net. So better get comfortable to refactor without tons of tests 😂
Testing, or "how to move the magic values out of your sight"
I once wrote 15,000 lines python script in one file.
I had to refactor thousands of lines at the middle of the development 😂
Great video once again, Arjan! Reminded me of a very good talk Kevin Henney once gave about the Gilded Rose Kata and refactoring, he called it "Refactoring Driven Development" :)
th-cam.com/video/kTcDBYCpj7Q/w-d-xo.html
You are going against today's dev mantra
We don't have to understand the system we just need to improve it - Jim Coplien 😅
Refactor code before it becomes legacy. Or discontinue it if it's not in line with product decisions.
With the size of existing projects, that's WAYYY easier said than done.
Code is a live organization, if it does not evolve, it dies.
Holding a fn key to use delete is very frustrating 😮💨
Step 4.5: type pytest 1 mio. times... 😜
yeah, my own code lolol
You've lost me there with all the very specific fixes.
This must have been a very tedious video to produce.
It was a lot of work to prepare, but it was actually really fun to do.
thats one ugly code