Wednesday, March 30, 2016

A few general tricks for simplifying conditional logic

Found it again. Placing it here.


Sunday, March 27, 2016

Safe legacy code refactoring

As soon as I was deep into the legacy code refactoring, the pinttests creation start to be a lot of pain.
It was a good exploration tool as well (and NCrunch rules for this purposes 100%) and following all the red dot's you can find a lot of interesting places inside the legacy stuff.

I knew upfront legacy code refactoring will be hard, but this requirement to have at least pinttests on top of it to start in some kind of safe grounds - this was really hard to achieve. Even if I have live system available locally for this purpose.

So I asked about my pain on some software meetings, and guys advised me to stick with this behavior only on the final stages, as soon as we have some pretty nice tools in our hands (like ReSharper, for sure) and we can make some easy refactorings without any test coverage and still have high level of confidence we don't break anything.

For example, if you have raw File.Exists calls in your code, you can easily extract method FileExists from it with the same parameter, doing the same File.Exists inside.
Then you can extract this (and maybe other File.* methods) into the FileProxy class just using ReSharper refactoring tool.
Then you can use old style wrapper around the local variable holding this FileProxy to initialize it if null, or you can use Ninject - whatever appropriate.
And if you have chain of constructor calls inside the same class, you can create a copy of the one you're using inside your tests and add this FileProxy dependency as the additional parameter so you can inject it from the test and use it in the tests instead of the old one.

Initially, I think it's not worth it to start faking this new possible context and going to get rid of the real files dependencies, as soon as in the pinttests context you're going to make a lot of setups just to make it work.

We can still use the same FileProxy in pinttests, but it will be ready for mocking when we're going to the real refactoring process of this giant 6+ screen method.

HttpContext faking pain for pinttest

Finally, got hands on the very old any huge legacy code method, when we have every dependency you can ever imagine: file system, statics, HttpContext, services, objects creation right into the method, DB calls via static methods, configuration parameters readings etc.

And the length of this code is 5 or 6 screens.
And we have very good if if if else if else else structure inside, code duplication all over, complex conditions. And on top of it try catches block in the middle who mostly silently catches all the problems (luckily setting error state 500 as the result).

As everybody knows, to start refactoring legacy code you need some tests coverage first on top of it, so you have a little bit confidence in what you're doing in the very beginning. But we're calling this code legacy for the reason. And this reason... lack of unit tests on top of it.
Hugh... some kind of recursion here...

So to get out of this recursion we need to cover this legacy piece of crap with some kind of temporary integration tests, who going to live just for the sack of initial refactoring. We name them pinttests. These tests supposed to run on the fully configured live system, on the real file system, real DB, real everything as much as possible to cover the desired method. And then we're going to chip things out one by one, refactoring problems and covering it with real unit tests.

So, pinttests! Horay.

File system is okay. DB is live and ready. services working as expected, all real objects created okay, statics make static calls - all good.
But we have one pain here. Old pain from the old ASP.NET web forms. And it's HttpContext.

To make things perfect it's a naked class. Just a couple of common interfaces implemented. And, finally, as the final nail in the coffin, it's sealed.

So when to start? Finally I found some example of creating empty HttpContext to pass in.

httpContext = HttpContext.Current =
            new HttpContext(
                new HttpRequest(null, "http://tempuri.org", null),
                new HttpResponse(new StreamWriter(new MemoryStream())));

And initially this fake works pretty well.
You can set cookies here

(thanks to Pavel Vladov for C# coloring)

httpContext.Request.Cookies.Add(new HttpCookie("SomeCookieKey", "MySuperValue"));

And you can set some cache values as well.

The first problem you're facing here is adding some staff to server variables.
If you try to make it like

httpContext.Request.ServerVariables.Add("REMOTE_ADDR", "123.123.123.123");

If compiles okay, but when you try to run it - failed. You just can't do it.

Some smart guys already solved this problem for us using another smart feature MS made for us: extension methods. Idea here: you can write some extension on top of HttpRequest object and it will be looking like

httpContext.Request.AddServerVariables("REMOTE_ADDR", "123.123.123.123");

And works great! (code of the two extension classes to make it work will be here later).

But... but it's not the final problem you're going to face.

In Response we have a method BinaryWrite() and we can't fake it because internally it checks the stream type he has in hands was created appropriately through the normal Response creation process. And we don't have it for sure in our artificial one.

And in my case this final problem actually switched my focus from pure pinttest to some kind of fair change without full coverage, but I think it was fairly easy and straightforward change - so we can carefully make it.

We're going to replace each HttpContext with HttpContextBase, as advised.
And in every public place, when we pass HttpContext in we're going to create sister method with HttpContextBase and call it from the old one getting the HttpContextBase via the new HttpContextWrapper(httpContext).