Refactoring Legacy Code STEP BY STEP (Part 2)

[@ContinuousDelivery]

If you'd like to follow along, the code that I started with can be found here: shitcode.net/348

[@ruixue6955]

1:32 - 1:36 reduce complexity + compose method 2:11 code demo: reduce *cyclomatic complexity* 2:58 favourite method to resolve this problem: *extract method* 3:29 great quote from *Kent Beck* : all of good designs are about *move stuff that's related closer together, move stuff that's unrelated farther apart* 4:22 *inline method*

[@sasukesarutobi3862]

Great video, thank you! I especially like the idea of Extract Method being a way of naming a block of code - it really helps communicate the usefulness of the refactoring in making code more readable and therefore maintainable

[@ContinuousDelivery]

Thanks, I am pleased that you found it helpful.

This was invaluable for me as well! Learned something really cool!

[@gonzalowaszczuk638]

Great video. I like to think about the "Extract Method" type of refactoring as a "Move 1 level of abstraction" technique, which you can do with Extract Method, but also do much more, depending on your language. For example, using C# as an example, I could do these in succession: Extract Method - Extract Class - Extract File If you extract method the block of code remains in the same class. What you do is you eliminate the coupling between the block of code and the free variables/scoped variables from the previous method it was in; however, your block of code is still coupled to the class members of that class. Your block of code still has visibility on the other methods; it still has visibility on the fields of the class, both instance and static ones, there is still coupling going on. If the above coupling is unnecessary, I'd do Extract Class, just for that block alone. And keep that class in the same file, it's simpler that way, nothing changes from the "outside". By doing this, you inject to this new class only the things your block of code needs, and nothing else. There is no more coupling between that block of code and anything else. Finally, if this is creating clutter and complexity in the file itself (i.e there are many different classes in the file itself and it's less readable), I'd perform Extract File to just extract those classes to separate files; perhaps even use a new folder structure for them too. Of course, there are more levels of abstraction above this. In the context of C#, there are namespaces; assemblies, and more. Theoretically, the same exact block of code you can continue "extracting" one level up in the abstraction hierarchy. First extract to a method, then to a class, then to a file, then to a new namespace, then to a new assembly/class library, then ultimately to a new package/project/etc. On the same vein, you can take such block of code one level down that hierarchy of abstraction, if it's more readable and maintainable to do it that way. I think some devs may get too caught up on only doing Extract Method and nothing else, but the above has helped me a lot in improving the modularity of a codebase, even when the public interface of what I'm refactoring (and its behavior) hasn't changed at all. The above depends on your language too, for instance, you wouldn't be able to do the above in Java where you can't have more than one class per file (at least in Java pre 8 when I was using it).

[@thebatu]

good content. Can you enlarge the code area in the videos plz.

[@ContinuousDelivery]

Next time

[@diegopego1069]

Thank you!

[@ContinuousDelivery]

You're welcome!

[@heathergray4880]

LOL when you saw all the breaks and continues I felt that

[@NilsElHimoud]

Haha! Exactly what I am doing most of the time at work. I 100% agree with everything. Of course there are refactoring steps that force you to do bigger changes and don´t allow you to keep your steps small. That´s why the commit after every change and successful test is important. The next step can be a big one that might fail and you don´t want to lose the small step. Change, test, commit slavishly!

[@ContinuousDelivery]

Yes, the "simple steps" idea is key. When combined with version-control it allows us to proceed much more safely.

[@csati]

Your videos are treasures!

[@ContinuousDelivery]

Thank you😎

[@Farren246]

I would prepend to the idea of moving related code closer together and before reducing cyclomatic complexity, that you should first reorder elements (usually methods / functions within a class) so that it follows a more consistent logical flow with initiation and main methods close to the top and the most commonly called methods from them immediately following. Doing this before diving into any actual code changes within the methods will set you up for better code and ensure you have a good idea of the overall function and flow before you start to change how it does the things that it does.

[@Soul-Burn]

5:45 Extracting methods is great, but "Don't worry too much about the names" is a recipe for confusion, like the game of telephone. 10:11 About the "Boy Scout" rule. Yes, if you touch an certain area, but not if you're touching things nearby but unrelated to the task at hand. 12:06 That's a good trick! That said, I'd make sure the input arguments are the same order in both functions. 13:14 I would not extract that part. The closing mirrors the opening at the top. Extracting it doesn't make sense without the the top of the function.

[@plouf1969]

Great video - I think there's way too few videos about refactoring code. I often observed that only the best programmers take the time to refactor their code (or other people's), and it always results in much great capability to extend or fix it. There's a point in your video where you skip a pretty important part, at 14:12, you take the jsonString out of the processElement list of arguments. This is among the most important kind of non-trivial refactoring: it's idendifying 'orthogonality', i.e. when you notice that the way a given argument is used inside a function is so trivial that it's not worth having it happen inside the function. An example I've seen are things like this (say in Python) def my_fn(client_id, client_name): out = dict() # populate out with stuff that depend on client_id, but that do not require client_name out['name'] = client_name return out That function was annoying, because looking up the name from the id was not trivial, and therefore it was frustrating that the client_name argument would be required given that it was not playing a significant role in the fn, yet appeared to. I just removed that last line, and looked at the calls to that fn in the codebase. There were only a handful of them, and none of them were doing anything useful with out['name'], or if they were, it was simple enough to just use the original client_name, rather than out['name']. This, in turn, prevented from having to do the id->name look up, and simplified the code even further.

[@OthmanAlikhan]

Thanks for the video, very practical advise =)

[@jean-baptistepotonnier2768]

I also like to remove the warnings, so that the remaining ones drive me to deeper, more interesting problems (we called it yellow hunting it one of my former team). With IntelliJ it's usually easy, and safe. I'm also not sponsored :P In Java, there is a lot of other small things which I think are helping, like try to make variable final and see if it still compiles, reduce scope of broad scope variables, sometime inline then re-extract at different points, etc. I would also avoid introducing things that bother me, like a close without open at the same level, or method that alter their arguments. So, I disagree a bit with you extracting closeJSON. But I guess it's a matter of priority, and we probably converge after a few refactoring iteration.

[@ContinuousDelivery]

Yes, I like to enforce the "warnings" thing on commit too. Turn-up the compiler warning-level to max, and then fail the commit on any warning. "Trust but Verify" 😉

[@insomniocode]

Fantastic!!

[@ContinuousDelivery]

Thanks!

[@Layarion]

9:25 If that loop had chances to throw errors, the test would Pass but fail to check if the error handling was still working?

[@Layarion]

14:36 I would personally want to extract that loop into another method because I don't right away know what it's doing from a glance.

[@OleguitoSwagbucks]

where is the full video?

[@ContinuousDelivery]

It is here: courses.cd.training/courses/refactoring-tutorial

[@OleguitoSwagbucks]

@@ContinuousDelivery Thank you for reply!!! That's where I looked yesterday. It has same three videos that on KZbin

[@ContinuousDelivery]

@@OleguitoSwagbucks Sorry, that's all there is.

[@OleguitoSwagbucks]

@@ContinuousDelivery Thanks man!

[@MrAbrazildo]

1:10, Java IS very nasty code! 8:40, I didn't get it: it still has the breaks. How passed this time? 14:12, I think you make too many f()s. Some thoughts about that: 1) Whenever you extract a f(), you are putting related things a bit away from each other, and a bit near to the rest of the outside code. This also can lead to accidental calls to those f()s. _/*Edit: and you'll deal with more code.*/_ 2) Compilers use to inline a certain amount of f()s. And if you raise that number, it can eventually get an unnecessary extra large generated byte code. This can lead to slowness. 3) 13:13, as I said on the previous video, for C++, just: *return jsonString.substr (blabla) + "]"* , which would be 1 line, no extra f(). But I guess Java is too bad to has that.

[@MrAbrazildo]

@dev null What a surprise! People use to get pissed off by my comments.

[@almostprofessionalrecords6651]

it has breaks, but in nested for, not the top one