Gilding the Rose: Refactoring-Driven Development - Kevlin Henney - ACCU 2023

[@codeRight]

Love this! It validates many things I tell people about refactoring and testing, things I suspect I picked up from the same sources (people & experience) as you.

[@ConductiveFoam]

Always a pleasure to see a talk by Kevlin. One minor nitpick: Starting with C#9 you can sort of emulate an if expression using a switch expression and relational patterns: var s = i switch { > 3 => "Bigger than 3", < 0 => "Smaller than 0", _ => "Default" }; Unfortunately this is constrained by the relational patterns only working for constants, but I think it's worth keeping in the back of your mind.

[@JonathanPeel]

I was also thinking about switch statement.

[@gustavnilsson6597]

That was fantastic! Great deal of laughs packed into this hour and a half. Thanks alot. :) The only time were I differ in perspective is about returning early. I think you can structure your code such that you never need to use 'else' if you refactor out a smaller method, then you can achieve the flow you want with 'return' and 'continue' etc. And this is simply to save the 2 indented brackets of the 'else'. But this is more of a style question than anything else really, in my opinon.

[@theondono]

When most engineers talk about "technical debt", what they really mean is they have "technical deficit", i.e. it's not that they have debt, it's that the debt keeps growing over time

[@paulr8992]

The table driven approach is what I do practice where possible. This makes the resulting code more clear. Next step for me with the Gilded Rose problem would be to wrap every Item in its own wrapper class (known as Decorating...). Every wrapper class can then implement its own updateQuality method. Btw: The code makes Aged Brie increase in quality twice as much when the sell-in date has passed. No mentioning about this in 'the spec'. Recon it is a feature?

[@aniketbisht2823]

1:01:55 I don't think item.Quality = Min(item.Quality + 1, 50); is the correct substitute for if(item.Quality < 50) ++item.Quality; as you're not supposed to change item.Quality if it is greater than 50 but the replacement with Min will clamp the values greater than 50 to 50.

Actually... at 10:15 : "The Quality of an item is never more than 50" so if it is above 50, then the new code fixes the internal state as it should not be over 50 ever as the specification states.

[@tropictiger2387]

1:04:28 This can be done in C# elegantly with a switch expression, but given that this is legacy code you might not have access to it.

[@peteobryan4650]

This was mind-expanding!That final step of moving all the business logic into a table is the Holy Grail. So good!

[@csIn84]

Or a switch statement. Less overhead, no iteration necessary.

[@TonyWhitley]

I've been a proponent of table-driven software for many years but I'm not convinced by the final version of the code. I don't like the mixing of data and code in the table and and the implementation is too fragile - conceivable changes to the requirements could break it completely. The result is more the outcome of a coding contest than code I'd like to be on the receiving end of, I'd almost be looking at the test code to understand what it was trying to achieve. Though I suppose that block comments explaining _what_ qualityAdjustments does and _how_ UpdateQuality goes about doing it would help.

[@codeRight]

I think the rules for Backstage Pass quality adjustment are the most problematic in this kata. At ~1:07:50 you mention that the refactored code has the same numbers as the requirements (10 days, 5 days) but the conditions don't match the text, i.e. " > 10" doesn't match "when there are 10 days or less" nor does "> 5" match "when there are 5 days or less" - that could be potentially confusing at first glance. You'd have to run the tests to make sure the behavior is actually correct. Also, the requirements say "at the end of the day, the values of sellIn and quality" are lowered, implying the adjustments are only done once per day. However, since rules are applied both before and after sellIn is changed when going from one day to the next, quality can actually be updated twice: right after we came into "today" and before we leave "today". These bring up some questions that relate to the correctness of the requirements and/or the implementation. I do like all the refactoring ideas discussed and demonstrated though.

[@sebastianmestre8971]

I had a go at the Kata before watching the talk, and my path was eerily similar to Kevlin's. There's something uncanny about watching someone else write nearly the exact same code you just wrote.

[@michaelrufenacht6279]

Pretty funny how OOD (and probably DDD) shaped my way of thinking about algorithms... all I see at the end is a factory and a polymorphic method call.

[@AldoInza]

C# has switch statement variable assignments now, and switch statements have a default case which is a necessary final else

[@edgeeffect]

I've always associated the rise of the word "paradigm" with the publication in 1980 of Marilyn Furgeson's new-age manual - The Aquarian Conspiracy.

[@shaunpatrick8345]

Ironically, the conditionals and return values at 1:04:34 could have been put into a table and processed by finding the first "match".

[@aniketbisht2823]

awesome

[@PairingWithDuncan]

I think it’s time for RoseCon! I’ll prepare a CFP

[@xcoder1122]

If there is a loop, that loops over many items and does a lot of stuff to those items, except for items it doesn't have to alter, I prefer if the first line of code in that loop is "if (...) continue;" as an early "skip to next item", over having the entire loop body wrapped into an if body and shifted another indention level. Also I prefer to split code into 20 small functions instead of having one huge 30 pages function. And if you have many small functions, of course you want to leave them as early as possible as that's where I can stop reading the function body. If there is a "if (...) { action; return; }" I know that if my object matches that condition, this one action is performed and that's it; end of code. I don't have to continue reading the entire function of if-elseif-else chains and loops, just to figure out, that none of these will apply to my object and I just read all that code to waste my time. More code is never better than less code if the code achieves exactly the same thing with exactly the same thing, unless more code achieves it faster or by using less resources and either one is critical enough to justify more code. Note: I'm not talking about code density. I don't say you should name "counter" better "c" as that is less code. It's not less code, it's just denser code. It's not about how you name things or how you arrange instructions, it's how many instructions you write and how many instructions you force other people to read and fully understand if they want to understand how your code works. I can read und understand 30 instructions quicker than 80 instructions, period. So if you want to write 80 instead of 30 instructions, you need to justify those extra 50 instructions somehow. 1:06:50 Not initializing variables is one of the top 10 causes of all times for catastrophic software failures and no, you should never rely that a modern compiler will catch that and at least warn you as it will always do so until the day it won't. Just check the bug tracking pages of modern compilers and you will find issues for every compiler on earth where the compiler did not correctly detect it under a certain condition. This bug has been fixed but how stupid does anyone have to be to allow himself to be fooled twice? Initialize qualityAdjustment to some value, always, that way it is for sure initialized, then change it as you have to. You can even initialize it to some value not even valid which gives you the ability to check in the code if it really was initialized (that is if it does not have that invalid value in the end). And my final comment: If you detect "Aged Brie" by always tying that string, just one typo in your code can again lead to a fundamental error. Don't repeat constant values, no matter if numbers or strings. Use constants, that's why pretty much every language offers those.

[@jasverix]

Did you watch the video through? He did in the end get to very much less code than in the start. But while going there, there was moments where there was duplicate code. That's pretty common refactoring pattern. Just duplicate the code, try it out, see where it gets you. If it goes nowhere, just do a git reset and try another approach. Continue is bad, but I do agree on the return idea. In the end, the if - else if - else part was ended by return every time. I normally wrap most of the loop as a seperate function, and use return. It is more readable and understandable than continue, it just is. Especially when going into multiple loops and needing continue 2; If that code causes a catastrophic failure, there is a missing test somewhere. But yes, you can elect to have some kind of "false" value, if it makes you feel easier. But as he said in the video, if some code fails because uninitialized variable, that code deserves to break. There is a state there that is not tested and not handled. If it is not possible to test all states of a variable, I think the code is too complex. Last, he was limited by the goblin. A better approach would have been to make subclasses of an abstract Item class. He does cover that quickly, but the rules of the game limits him from doing that.

[@casdf7]

I also did not like his take on the uninitialized variable. It is mutable in the whole context. I think this is just bad. And i do not know what speaks agains making a function that just returns that value so it is instantly initialized and immutable.

[@casdf7]

also his variable is defined outside of the scope where it is used. Absolutely catastrophic

[@Coburah]

25:00 If Kevlin doesn't like "continue", then he probably have something against guard clauses as well. I use continue as guard clauses to skip an iteration of the loop early (similar to early return). Edit: oh, okay... He addresses guard clauses directly after the remarks on "continue". I strongly disagree with him. 😅

[@cod3nam3]

If this talk would have been code, it would be considered legacy and in need of refactoring. It is all over the place and does not get to a point, but jumps around from topic to topic. I mean after 30min he completly forgets about the kata he was explaining, makes an Intoroduction that should have been at the begging, reads comic strips, suggests books and then gets back to the kata.

[@MortenHoustonLudvigsen]

As others have suggested: with modern C# one could use switch expressions. Something like: public void UpdateQuality() { foreach (var item in Items) { var (qualityAdjustment, sellInAdjustment) = GetAdjustments(item); item.Quality = NewQuality(item.Quality, qualityAdjustment); item.SellIn += sellInAdjustment; } static (int qualityAdjustment, int sellInAdjustment) GetAdjustments(Item item) => item.Name switch { "Aged Brie" => (item.SellIn (item.SellIn switch { -item.Quality, 3, 2, _ => 1, }, -1), string str when str.StartsWith("Sulfuras") => (0, 0), string str when str.StartsWith("Conjured") => (item.SellIn (item.SellIn qualityAdjustment switch { 0 => quality, > 0 => Min(quality + qualityAdjustment, Max(quality, 50)), < 0 => Max(quality + qualityAdjustment, Min(quality, 0)) }; }

[@ABaumstumpf]

Continue is not bad - not understanding when and how to use the tools the language is providing you is bad. If you have various pre-conditions for when to actually process and item in a container you need a way to skip the item if it fails the conditions. Now if we are supposed to not use continue then you must provide an alternatice..... how about your also dreaded mountain of if-indentation? No? then.... what options are left? You tell us not to skip the items that must be skipped, you tell us not to encapsulate them in if-blocks... goto?

[@jonathansaindon788]

Sandy Metz talk is way shorter and gets to the point faster. You are losing me here

[@adm3333]

I agree that the concepts presented in Sandi Metz's are better, as are her presentation skills. However, she did also actually refactor the Normal Item completely incorrectly. This provides a fine example of why you should actually refactor things instead of rewriting them from the tests.

[@myronww]

I sort of dis-agree with his assertions about needing to use else statements. If you look at the way the compiler generates codes for ifs, it generally reverses the logic so that the optimal case, without a jump, is to fall into the if. If you have else statements, you are jumping. With a processor that is pipelining instructions, the if case is the optimal case. if (something) { if (another_thing) { } } The case without jumps is to go into the ifs

[@erikgrundy]

Kevlin's point has nothing to do with what assembly a compiler generates. He is asserting that people are writing code as if they are writing in assembler, when we have collectively agreed to code at a higher level of abstraction than that. This isn't about performance, it's about readability from a human perspective.

[@craigyoung8008]

1. Don't try to second-guess the optimiser. Let the optimiser do it's job the way it sees fit. 2. Your code should focus on *human* readability. 3. Even if you don't write any code for *else* condition, that condition still exists as a possible flow. A flow that effectively performs a NOP. 4. The point about using *else* is that you are _explicitly_ considering the otherwise implied "if == false". When it's missing, the future reader still has to think about it - without the visual cues.

[@TonyWhitley]

I sometimes put the else in as a comment _if (x in complicatedExpressionDefiningSomeGroup)_ _{ ... }_ _// else x is not a ..._

[@Zeedox]

I agree, code without else (generally using guard statements) is more often than not cleaner and clearer.

[@khanra17]

C# 🤮🤮🤮🤮