Refactoring Legacy Code: STEP BY STEP (Part 1)

I had actually never thought of doing something like approval testing as a stop gap measure until you refractor the code enough to be able to test it properly. Very useful tip indeed.

[@HuyVu-vi4ut]

Man! this video has really good content but the way you show the screen of code is difficult to read. Pls change it to normal screen as many other youtuber if you can. Thank you

[@gonzalowaszczuk638]

I heard it called "Snapshot testing" in other articles/resources online (instead of "approval testing")

[@edgeeffect]

Thanks so much for adding more words to my Google-fu. :)

[@MatteoRegazzi]

Thanks for the video Dave. I usually love to see this kind of video also on my iPad beside my laptop but this way I cannot see code, it reminds me I’m ... ehm ... getting old :-). Great topic.

[@ContinuousDelivery]

Yes, I was torn between coding in "presentation mode" and at a more normal resolution. I thought that there was some value in seeing the "shape of the code" which you tend to loose in presentation mode. I confess that I didn't think about watching this one on an iPad :(

[@MatteoRegazzi]

@@ContinuousDelivery I also prefer the standard mode especially during refactoring legacy because the shape can reveal structure: clouds of code can reveal logical blocks. Luckily I have also a desktop monitor beside my laptop :-D

[@Layarion]

@@ContinuousDelivery ya this is a problem i face when I make video-game tutorials on YT as well. I usually just end up erraring on the side of small screens by setting my desktop to 720p and record at that, then call it done. one thing i hate about my approach though, is YT doesn't treat videos under 2k resolution with the same respect - meaning they give less bitrate ratios to those videos. aka, under 2k res videos have noticeably more compression.

[@Mozartenhimer]

I wish I had seen this 2 years ago. Sadly I've learned a lot of this the hard way.

[@ruixue6955]

2:36 refactor technique: *approval testing* aka. *characterisation testing* 3:23 4:40 what happens when you run *approval tests* 4:43 captures the output from the code that you call 5:50 approval test is a very useful tool during refactoring when the whole point is to avoid changing the behaviour of the code 6:06 other thing need to do: run approval code with *test coverage* 7:46 step 1 of refactoring in the code demo: *reduce clutter* 8:40 rename method arguments 9:29 9:43 rename method 10:50 IMPORTANT practical tip: focus back the simple steps and refocus on the task at hand 11:23 14:34 commit with small refactor 16:01 takeaway: *work in small tiny steps*

[@NilsElHimoud]

Thanks for the hint to approval tests.

[@ContinuousDelivery]

Pleased to be of help, I missed their value for a long while.

[@ruixue6955]

Just notices today that approval test is the also called characterisation test, which was discussed in Working Effectively with Legacy Code :) shame on me that I didn't realise the importance of it when reading the book :D

[@George-or3uv]

I’ve been looking for KZbin videos like this.

[@JohnWasinger]

Check in comments that I hate: “Fixed re-indexing problem of the stem bolt conflagers. Fixed mixed use of tabs and spaces in the source code to improve readability.” Thank you for fixing the code’s readability but please make that a separate (no change in functionality) check in.

[@Glinkis]

Yes, each commit should ideally be described by a single short sentence.

[@MarekAndreansky]

The slightly tilted text editor is not fun to read.

[@ContinuousDelivery]

Sorry about that

[@MarekAndreansky]

@@ContinuousDelivery Its fine, I got used to it after reading for a bit. Thank you for the interesting content!

[@Layarion]

13:19 I feel like you should have left in that block saying "dead, left it here for compatibility reasons" or something - because someone else looking at the block might be wondering why it's still there.

[@TheOceanLoader]

Yeah I agree - mark it as "obsolete".

[@MattHeard]

Thanks for the video. I think that this example class had quite a shallow tree of conditional branches, so the video didn't quite demonstrate some of the additional steps that are sometimes necessary with characterisation testing. Getting 85% line coverage with a single test implies that most of the code is quite linear, but often legacy classes have too many responsibilities and too many conditional branches. I've done characterisation testing where I can only get 10% line coverage with the first test, with diminishing returns for subsequent tests. Any advice on how to determine what kinds of differences in input to use when trying to get greater coverage before de-cluttering? What about when the code is highly coupled to something unreliable like a database or a network connection? Thanks again for the video. 🙇‍♂️

[@ContinuousDelivery]

Inevitably any code where I can demonstrate the techniques in a reasonable amount of time will be simpler than a real, nasty production system. I picked this piece of code as a compromise, it is bad code, it is complex enough to require some discipline to change it safely, but it is enough to demonstrate how I tackle this kind of problem in an hour or so. The coverage I got surprised me too, it was better than I expected. My approach to approval testing is to do a few tests that push the code through a few different paths. This code didn't need more than one. The next videos in this mini-series will, I hope, demonstrate the techniques, as I say in the video this one really only covers the pre-work. Keep an eye on my channel for the next two videos and let me know what you think.

[@MatteoRegazzi]

Good point. I think you have to read code not covered, then try to test your assumptions adding tests and variations on input. Maybe the component you're working on isn't SRP compliant, maybe it's just convoluted, these are some reasons you are reafactoring (of course, in order to get the code readable/tetstable/maintanable). Seems to be a chicken-egg problem, but the first characterization test (or a Golden Master test) will be your chicken, so you can start to test your assumptions having a sefety net, even if with it is a net with very wide links that you are going to shrink.

[@MattHeard]

@@ContinuousDelivery Thanks for the reply. I'm really looking forward to the next two videos.

[@ContinuousDelivery]

Yes, exactly. Actually in this case I did that by varying the content of the XML file, but stupidly, I didn't record that part of the exercise. Doh! I started off with lower coverage, I think it was 30 - 40%, and then bounced between the code, to try and identify what input data may push the code down different paths, tweaked the XML and tried the test again. All the time tracking the coverage.

[@MattHeard]

@@ContinuousDelivery I think this intuitive input-tweaking is the hard part of characterization testing that I find difficult to teach. I would love to see a video where you mess around with test inputs for 10 minutes. 😄 It would be very informative.

[@OthmanAlikhan]

Thanks for the video, very practical advise =)

[@Eamo-21]

This is very useful. Thanks Dave

[@kinddata]

If am using an untestable legacy platform like vb6 & stored procedures. Should I bake a self tests into the main code?

[@ContinuousDelivery]

There are some references to unit testing VB6 so maybe worth bit if a trawl with Google. I found this on a quick look brainbaking.com/post/2016/12/vb6-unit-testing/ What I have done in the past, when working with older tech, is to write my own, simple version of (x)Unit. It isn't all that hard if you keep it simple. I have seen some unit/functional tests used for Stored-Procs, not ideal because the DB is there in the back, but better than nothing. Good luck!

[@kinddata]

@@ContinuousDelivery thank you. Its inspiring to see what others have done with vb6. I think I will try make one class called TestPoint inside the main code, and get a test runner to call it often via, activeX createobject. I look foward to your next video.

[@matthewtrow5698]

Great video, but there's a few other options to consider: 1. Run away. 2. Hide and hope someone else picks up the end of the stick with poo on it 3. Look for a new job 4. Rebuild it from scratch 4 is the best option, if you are afforded that luxury - you are then in the realm of "green field" development, everyone's favourite, plus you can make the same mistakes all over again ... (joking ... I think) ...

[@edgeeffect]

3 is the best option, if you do 4 they'll only break it again.

[@rich63113]

Rebuilding small chunks of code is usually fine - but rebuilding a legacy application is almost always a giant expensive failure - because yeah - because any legacy application has a ton of hard fought fixes in it, and undocumented features.

[@Warpgatez]

This video is really well done minus the slanted preview screen. It’s disorienting and the animated background isn’t needed. More distracting than anything. It would be better to just have the full IDE take the screen and your camera a little smaller in the bottom right over the IDE.

[@JorgeEscobarMX]

Code text is way too small. But a great video so far,.

[@diegopego1069]

Thanks for the video!

[@ContinuousDelivery]

You're welcome!

[@Layarion]

CD, i'm a dumb C# noob and aside from fiddling on a personal project and Unity, that's my only experience. My point being, I wanted to try and do this myself with the code you provided on Git before watching the video...but i download the ZIP, and have no idea how to even navigate the file structure, find what i'm looking for, or even what i'm looking for without a VisualStudio project file to open.

[@wvvwwwvvw]

I wonder whether I would make the old code base as a branch in git?

[@ContinuousDelivery]

I wouldn't, refactoring is behaviour-preserving, and is best done in very small steps. So you should be safe to release at any point, or not very far from a version that was.

[@ajmeyer66]

When I think of "legacy" code I tend to think about older languages like "COBOL",BASIC,"DBL/DIBOL". Procedural style languages without the notion of "Objects/Classes". That is a whole other world. The current language I am working with (Synergex DBL) does have the notion of "Classes" but the code base was developed prior to that feature and essentially doesn't use it. Some newer code that has been introduced does use "classes" but 99% is simply subroutines and lots of goto's. Very difficult to refactor and generate "tests".

[@edgeeffect]

I'm dealing with "PHP-3 style" PHP-5 written by amateurs.... I DREAM of a nice, simple bit of CoBOL to get my teeth into... I used to LOVE CoBOL!

[@MatheusAugustoGames]

Characterization tests may be the way to go in your situation, given the lack of structured programming. What tooling do you have available to you? Testing and code coverage are great tools to have on your toolbelt. Depending on your refactoring tooling, reengineering may be the way to go. Also remember to use static analysis tools -- specially on older codebases!

[@ajmeyer66]

@@MatheusAugustoGames Unfortunately not any really. Code is essentially run in a Linux Terminal screen, old "Green Screen" VT terminal emulation. Just a compiler and linker, multiple code base instances, development, testing, user acceptance testing, production. Most developers used VI editor to do the actually coding, VERY old School. Since I am a contract programmer my mandate is to simply work on modifications requested and to do it as quickly as possible, no "redesigning" the world. Not worth it to me to rock the boat, a few more years to retirement so not much desire to fight the bureaucracy anymore.

[@MatheusAugustoGames]

@@ajmeyer66 That's very unfortunate for the coders that are gonna come after you're gone. I bet that if you tried, your talents could be used to make their lives easier by leaving the code in a better state than when you arrived.

[@ajmeyer66]

@@MatheusAugustoGames I do my best to refactor as much as I can. I have also built a few "Utility" functions and created documents to describe how to utilize the utility functions. Whether or not the utilize the tools or not is out of my control. We are however developing some "AutoGen" scripts that will read the source code and then build "wrapper" programs that can then call their built up library of functions. This will allow us to create in-house test harness programs that can call the utility functions via the "wrapper" program and pass test parameters in and evaluate the responses given back.

[@Farren246]

Great to view older videos and see your own continuous improvement. Lots of background hiss in this one, poor green screen and poor voice quality, some fixed by 5 months and all fixed in recent weeks.

[@ContinuousDelivery]

Yes, learning lots about video, sound and editing on this journey :)

[@11rchitwood]

Do you only commit when the tests pass?

[@ContinuousDelivery]

Yes

[@MrAbrazildo]

7:30, I use tab = 2. 9:10, what is xPathString? You just deleted the comment. 10:15, don't delete, just put it at the right side. 11:00, mm... I wouldn't do that. 11:11, omg... I know Java is awkward, but the comment was saying where things were placed. 13:52 and 14:01, somebody put this man in jail! :) 12:25, I would permanently highlight the 'hasChildren' 1st, or if this doesn't exist in the IDE, I would use the find recurse, to spot every occurrence of it. 12:45, "equal to" and "has it" are 2 different things to me. That's why I would not delete the comment. - Here's the proof of a DRY violation: there's the hasChildren f(), but the programmer forgot it, because the previous f() was too big. - 13:17: at least in compiled languages, deleting (safer: turning it into a comment, actually) getXPathString() would reveal all calls to this "f()". How C++ is elegant compared to this s**! You could write: 11:20, jsonString += Str1 + Str2 + ... StrN. 13:00, return jsonString.substr (blabla) + "]". Period! 13:40, String tagetString = shortXPath.equals("") ? "//toc" : "//".

[@istovall2624]

Ctrl+shift+a Del

[@tristanstricker967]

Code is tiny. Please make bigger

[@ContinuousDelivery]

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

[@ContinuousDelivery]

@@gezenews I don't think these have gone out of date. My production values have improved a bit since I recorded these, but the content is still good I think.

[@vrjb100]

Don't refactor code in this way. When you did keep up your knowledge up-to-date, 99% of so called seniors don't, then you would know the use of a json library does the same but better. Because you don't know the existence of a library doesn't mean you should write one yourself.

[@ContinuousDelivery]

I am afraid that you are missing the point. This exercise has nothing to do with what the code does, this is how you tackle bad code, to reshape it to be better, and testable. This is about the skills of being a software developer, it is equally applicable to the code that people that assemble 3rd party components together, as it is for the people that build custom systems, or build the pieces that you assemble together.

[@edgeeffect]

" keep up your knowledge up-to-date, 99% of so called seniors don't"... that made me laugh SO loud.