Solving Gilded Rose Kata (No Nesting) | Refactoring Challenge #2

i'm ray myers welcome to craft versus cruft we've got another refactoring challenge and we're gonna do a little something different today we're going to do an actual refactoring kata uh previously i've done an obfuscated c doughnut which is actually my most popular video on this channel and then we refactored someone's first project they had ever done so those weren't things that were deliberately created as refactoring challenges this is this is the gilded rose refactoring kata and it is just a magnificent exercise is popularized by emily baich uh though created originally by others and you'll find some some links in the description to that and some of her other work and i think this exercise is is beautiful in just its frustrating realism uh the way that code tends to just get muddled up over time if uh features are just sort of added naively as we go is uh very conducive of this so let's go ahead and look at this exercise real quick this is where you might go if you were finding it you can choose your language of choice though um it might be a good idea to do it in its uh original tongue or or something closely related we're gonna use java today it was originally in c sharp so this is the um kind of canonical description of it but the general premise is as you know we are a small inn with a prime location in a prominent city ran by a friendly innkeeper named allison we also buy and sell only the finest goods unfortunately our goods are constantly degrading in quality as they approach their cell date we have a system in place that updates our inventory for us it was developed by a no-nonsense type named leroy who has moved on to new adventures your task is to add a new feature to our system so that we can begin selling a new category of item first the introduction all items have a sell in value all items have a sell in value denoting the number of days we have to sell them all items have a quality which denotes how valuable they are at the end of the day the system lowers both of those values every turn all right so that seems pretty simple but then there's lots of um exceptions and special cases here as you often see in the real world like once the sell-by date is passed the quality degree is twice as fast uh quality of item can never be negative age debris is a special item that increases in quality the older it gets the quality of the item is never more than 50. uh sulfurous being a legendary item neither has to be sold nor decreases in quality backstage passes like aged brie increasing quality as its sell and value approaches quality increases by two when there are ten days or less and by three when there are five days or less but quality drops to zero after a concert we have recently signed a supplier of conjured items this requires us to update our system so this is the update we have to make conjured items degrading quality twice as fast as normal items so uh there's one more caveat we cannot alter the item class because it is protected by a goblin so um that sounds really easy right like as so often things do we just have we've got all these rules and then we have to say that now there's conjured items and they decreating quality twice as fast so let's take a look at the code and if you've if you've done a lot of industry coding you will kind of recognize the conundrum where we're in this is as i say frustratingly realistic um because they're just you know kind of special cases within special cases there's quality being updated in a lot of different places like up here and way down here and so it's really kind of confusing to know well what uh what do i do in order to you know make this one change about let's see conjured items degrading and quality twice as fast fortunately the original exercise provides um a a file with a kind of a regression testing suite some some scenarios and then how we expect things to progress and while there aren't uh targeted unit tests that give a little better feedback i've gone ahead and found some and and adapted them and that's uh if you go to my repo that is going to already have these unit tests in or you can create them on your own that's where a lot of people start with this exercise is they'll say okay well first thing we have to do is add these targeted unit tests that directly capture all the existing requirements that is part of my solution here i've just done it off camera and it's basically just one corresponding you know that sets up each individual scenario we we have here so i think that's fairly mechanical um there's plenty of literature on on how to do that sort of thing if you'd like to learn about it before you watch me try to muddle through a solution here i would highly suggest you know taking this exercise going off and and giving you go yourself and and everyone's going to come up with a little bit different approach here because what we want to do is you know we want to make this code a little more workable we don't just want to make it like even more complicated than it is in order to solve this so go go off try it is it's a great exercise but we're back now and i will i will go ahead so as i said i've added unit tests let's go ahead and run those and this text test fixture has uh doing the kind of end-to-end regression can see if we look at the differences that it's only failing right now for this conjured item which we haven't implemented yet so that's exactly what we expect and then the other targeted unit tests they're all uh passing you know except for the conjured ones so i think everything is going well so far now the first thing i see is like this method is is definitely i mean it's too nested it's too complicated but also it's too long the the problem is though that like because the same values are kind of being uh modified at several points in this it's very difficult to find a part that you can meaningfully extract no normally i would just come in start extract method extract method i'm not gonna get too far with that uh today you know we're going to have to get a little more uh creative in order to do things though i do see at least one level of nesting i can take out and in my solution a goal that i'm gonna have is to actually have no nesting because too much nesting is a big thing that's wrong with the existing code i'm gonna say every method is gonna nest no more than one level so if there's an if statement in a in a method nothing else can nest inside that no ifs and side ifs no ifs inside for loops uh one nesting level per method and it's gonna end up being very flat so we'll see if i'm able to accomplish that but i believe that can always be done so i do see though one method i can just naively extract here which is the the contents of this this loop so let's just say i want to take this uh extract this item into a variable not just that occurrence let's do them all nice because the items don't really relate to each other you can you can localize these to only applying to one item you can line that there all right so are we at least no more failing than we were before we're good and also this is just a simple iteration through a list so i think it'd be more appropriate to do it with a java 5 style for loop at very least there's more advanced things you could do too but it's it's not that fancy of a situation all right so all the existing requirements are still passing commit so i'm seeing a lot of magic constants here and that's a thing that's uh that's going to bug me so i think we might as well start to extract those and you know maybe these should be like item name h3 or whatever i'm just gonna extract them by a trivial name now and and we could uh always come back and decide on a convention for all of them do backstage passes i didn't replace them all for some reason all right we're still in good shape there yep so as we step through this i'm seeing uh these guards around if the quality is greater than zero we can subtract from it if it's less than 50 we can increment it and we're checking that it's less than 50 again that it's less than 50 um and similar thing going on with this uh with here and here so there's a lot of range checking you know whenever we decide to decrement the quality and i think that could be um that could be handled in some some better ways there's a couple ways that come to mind i could now remember we can't uh give item a special method that says decrement that you know encapsulates the range checking because it um because of the requirement you know the goblin is stopping us from modifying that one but we could make another method that uh that utilizes it and another thing we could do is we could do all our modification here and then at the end have a way of uh you know if it's gone above or below the range to bring it back in and i'm not really sure which i like better um if we were in the same room i would say like you know what do you what do you think should we make the uh you know adjust quality method or should we uh just add to it subtract to it in place and then afterwards bring the the range in but i'm going to try the um the first one and i'm going to say this is like let's see this is an adjustment and that this is adjust quality is what we're doing here all right so it's asking me do i want to do this in other places yeah i think i think we can do this we can apply this elsewhere it didn't catch that this minus 1 also could be applied in that way but of course it can now we haven't actually changed anything yet because we haven't done any range checking but we'll be able to do that because we're uh we're doing this so just just you wait so this would be adjusting the quality by minus one [Music] similarly there all right let's make sure that yet things are as good as they were now i claim we can just enforce the logic we know from the requirements that if item dot quality plus adjustment is so this would be like the new quality right that if the new quality is less than or equal to 50 and new quality is is greater than or equal to zero if it's in that range then we can make that adjustment otherwise we will do nothing and that should be fine if we want to be totally explicit here we can even give his invalid range its own name there up found another one that we missed before uh but this one this one's adjusting it to um zero well that's that's interesting in and of itself but we'll we'll get to that so first off um we didn't break it no we didn't break it so now i think the idea is this quality less than zero check this qual or equality greater than zero this quality less than 50 that's no longer needed i'm going to inline that variable and this one as well maybe even this one let's see what's what's all the side effects that occur in here adjust quality and adjust quality and adjust quality so i think uh i think we lose that one as well see fortunately we've got very comprehensive uh automated tests here so we can um be pretty confident here in making these uh simplifications that aren't all supported by the automated tools all right yes so all the things that are supposed to pass pass still and we've removed quite a few uh of the the things that have been complicating this let's uh let's look at this diff like move nesting there there there there like this is great so remove um or i'd say what we've done here is consolidate um bounds checking on quality something like that and you know we've got a nice little atomic commit there um now when it was doing this it's actually it is adjusting the quality but it's adjusting it like by the negative quality itself it's setting it to zero um i guess we could use adjust quality for that you know it's kind of up to you whether that's a uh a misuse of it but um we've now removed enough complexity that if we wanted to we could probably justify trying to add the complexity of uh this conjured item uh handling so do we want us keeper factoring before we get to that or do we want to try and add it now i'd go either way let's try and add it now and see how difficult it is and if it's uh if it requires you know a little too much uh modification still then then maybe we'll uh we'll try something different so recall we're trying to do is make conjured items degrading quality twice as fast as normal items so how do we know it's a conjured item well it's called conjured mana cake i guess um anytime we do grading quality if it could be a conjured item if it's a hundred item then we want to do that by twice as much okay so this is one place we degrade this is another place we degrade so there's two places and this is if it's not aged brie and if it's not backstage and if it's not sulfurous okay and then there's this if cell in is over zero we're degrading at twice the rate okay one thing what i'm trying to avoid doing here is saying okay if it's conjured degrading you know by two otherwise degrade by one and then doing the same thing here like that's uh that seems like you got kind of spooky action at a distance it's easier it's easy later to update one and forget to update the other um you know we kind of got a lot of these these patterns here what if i extract this this minus one here and say there's a degrade rate and that degrade rate actually is is dependent on something right for example if the name is conjured then degrade rate is two otherwise it's one how would that do for us let's try running the test hey all right and for the first time we've got including the uh you know test that didn't include that requirement we've got a green bar now so that's that's really cool let's uh let's commit right there so not only have we implemented additional requirement but this this code is significantly not drastically you know because it's still got a pretty long method here but significantly better but um i promised you that i was gonna be able to reduce the nesting level you know down to one per method and we've increased it by decreased it by a few but i don't think we've quite gotten nearly there so let us see what else we can do to uh to go the extra mile here so i'm seeing aged breed backstage passes sulfurous aged h3 backstage passes sulfurous these are both being checked like all three of them in order to determine if we should degrade so and perhaps there's a concept here like i guess those are the three items that don't degrade or they uh you know because backstage passes the other two don't degrade it all but backstage passes they you know um they go up or they stay the same until they just go to zero so they don't follow like the normal degradation so i wonder if you could do something like if it degrades right just a grade equals it's uh not aged pre and it's not a backstage pass and it's not sulfurous and then we could say like let's take this out then and say if it degrades then we've got this degradation otherwise this is now an if that only has an else so we can invert it and i think there may even be a uh yeah a thing there and see what happened there it was a not a and not b and it converted that to a or b the knots go away the and becomes an or you can do that in in either direction with ants and ors that's called de morgan's law it's uh it's a nice little piece of boolean logic when you're simplifying code uh you can look that up de morgan's law probably done that manually many a time when uh modern ids could do it for me great so now that's like another if within an if we don't have anymore right so we're we're flattening it and if it's a backstage pass you know we already know what it is so we don't really need this additional check so i'd say this can move out of there and kind of be its own logic let's see where if we're still doing good here all right flatten ifs now what has happened here if it's age debris or it's backstage passes we increase the quality and then if it's backstage passes then we um do like yet another increase well that i think would be a little simpler if we just had the backstage pass handling in one place and debris handling in in one place still again yep okay backstage pass this still kind of irks me we got three different quality adjusts and it's like one of them or two of them or three of them are going to get called but we can come back to that so now the the point of most nesting my public enemy number one here is this guy so this is all if sellin is less than zero so if it's expired um so maybe we could just do that huh we'll see we'll see we're passing in the degrade right i don't love that and so if it's not h3 we do the normal stuff otherwise we adjust the quality upward so that's like having the not case be the if and the positive case be the else is a little weird so i'm going to invert that but maybe we can um just yeah let's let's actually uh in line this this method back because i want to use this does degrade thing i want to use a little more so i think we're not quite ready to extract pieces of this so what have we got here we've got if it's not age debris because we're in the else and then if it's not backstage passes and if it's not sulfurous we're going to degrade what is that well i recognize that that's just does degrade again so we could take all of this and say if does degrade adjust quality to grade rate that's pretty intuitive and then this goes away and then we can invert this if there how we doing great yeah this is this is starting to look much better so we've only ever got two nesting levels um oh no we got one with three i guess we can we can cheat then it's back to two nesting levels we're doing a lot better but i'm seeing what they call sometimes synchronized ifs i've got the same like if does degrade over here and if it does degrade over there that's um that can be confusing that's a common uh place where you can introduce bugs if you then you know where to change one and forget to change the other kind of spooky action at a distance let's see that's like the next thing that's jumping out to me something else might be jumping out to you but i wonder if we can say the degrade rate actually just is what changes here do you see what i'm saying and being a little more clear let's say we've gotten is expired and i'm moving it before the sell-in was decremented so this is a little subtle i think i would have to uh do that instead let's say that i missed it let's see if the unit tests catch it but where am i going with this where are you going with this ray well we already do the degradation once can we do it uh only once instead of twice and just modify the the degrade rate that's what i'm saying so for example if is expired double the degrade rate i don't like that we're you know we can't make this final now i don't like that we're changing the degrade rate but kind of one thing at a time here we'll get there we'll we'll be able to just set it one time i think all right so this is all screwed up but if we dig into this i wonder if we'd find that the expiry date's a little messed up and if we move that yes all right to account for the fact that we're checking expired before we decrement sell in and not after then i think we're still okay i promised you i'd be able to prevent this degrade rate from getting modified i'm going to say that this is you know something we can just pop into a little method here and say let's give this like a base to grade rate call that and then we could say if it's expired then we return two times the grade rate otherwise we return just the base to grade rate unmodified i like times two instead of two times i don't know about you what about that so you know if you want to use uh is instead of turner is that's just fine but um there we go so now like nothing's being modified everything everything's final what i might be trying to prevent there's there's no chance that by like moving the order of some stuff around later i'm gonna accidentally break it are we still good yes uh so what the heck did we do how do we describe that consolidate degrade logic let's say um okay so this is this is kind of nice if it degrades and we've computed a degrade rate and whether it should degrade if it degrades then apply the degrade rate if it's age debris then we um we don't degrade we improve if it's batch stage passes we do you know our weird backstage pass adjustment um that's now kind of independent of other stuff i think oh not not quite let's uh in line that back i just saw oh yeah there's some other backstage pass uh stuff here but uh i think it's now clear enough how we might apply that because we've got is expired so we could do some is expired handling here right and keep in mind the computation is set to be like we'll know for that throughout the entire method because we're checking against less than one so it doesn't matter that we haven't modified the selling thing because it's not being checked at this point this is a little more independent we just we already know it's not age debris because it's a backstage pass so so no need to uh carry that that part of the if but this is expired that is gonna be necessary um it is a backstage pass that's so if you followed all that this should be equivalent i claim um of what was there before is just to within this if check is expired great now handle backstage pass update backstage pass quality something like that just to be consistent with the name of that method let's say this now you know like i said earlier i'm not in love with the fact that we've got like several different calls to adjust quality i think this could be simplified a little in terms of handling these these windows in a more intuitive place these uh you know the selling dates and so forth but it doesn't register high enough that i that i care anymore because it's all in one place and um there's not a lot of nesting so i'm leave it be for now consolidate backstage pass handling cool checking if it's expired and it's age debris there's this special handling here and that now like there's since there's not extra you know kinds of logic and else's and everything that there was before there's not an automator factoring there which there probably should be but there we go good if it degrades adjust the quality by the degradation rate if it's age debris we want to adjust the quality we increase it if it's a backstage pass then we do a bunch of uh specific backstage past logic to update its quality if it's anything other than sulfurous we adjust the cell in we could give that kind of a a name here like um has sell by date something like that we decrement the cell by date if it's expired and it's age debris then we adjust the quality by one so this is really both some age debris handling so to me these kind of belong together i don't know what you think but let's see we've got like an adjustment and to me it's like that expire that adjustment is either two or it's one could be a way of handling that still good and you may be noticing that i'm starting to accelerate you know i'm starting to be able to make changes with less and less thought as i as i reason about this because this code is just getting more easy to reason about and that's uh that's the thing about refactoring you know is if you're doing it well if you're if you're doing it in a way that like really enhances you know the conceptual um obviousness of the code the simplicity of it then um you start to get those returns back very quickly so now um you know i can rearrange these any which way whereas before the code was very order independent so just to make the point i'm just going to like scramble the order of all this right and i think that the test should still pass yeah whereas before you wouldn't have been able to do that so like this is this is much easier to work with now and i don't even i don't even intend to do that change so i'm just gonna blow that away bring it back so look at this we've got you know you could uh you could quibble about like should we um you know there's 20 lines should we extract a few things here and there because they don't depend on each other anymore we could extract pretty much all we want the longest method is uh you know around 20 lines we could probably get that down um but i think this is this is pretty good right now i'm pretty happy with this and as promised we were able to get the nesting level of every single method down to just just one and if you want a quibble you know we've got um kind of a hidden if in an if with this ternary inside of an if but it should be pretty clear that we could um extract method ourselves out of that conundrum if we uh if we wanted to so i think this is very flat and this is very um maintainable it's not you know there's any number of other ways you create abstractions um that you might like better but i'm pretty good with this so thank you for tuning in um let me know what other kinds of refactoring exercises and other industry topics you'd like to see covered and if you've got a particularly interesting solution of gilded rose as well leave a comment subscribe if you'd like to see more it also lets me know the people want to see more so that's very good to hear always and remember what laozi said in the dow jing with patience the most tangled chord may be undone thank you

Channel: Craft vs Cruft

Views: 1,044

Keywords: software engineering, java, legacy code, refactoring, coding, tutorial, kata

Id: 5oAs5Jr5njU

Length: 41min 14sec (2474 seconds)

Published: Sun Apr 11 2021