In the previous post we’ve removed some of the technical debt that could be found in our NetDeveloperPoland Website application. In this one we will remove it even more. We can even maybe reach a B? Let’s see where we’ll end up at the end of this part.

Before we jump into the code one thing to mention to the previous one. There’s another way to fix NDepend raporting ‘not-used’ method on the ones that use dynamic. We can use and attribute [IsNotDeadCode] to mark such method (it can be a custom attribute, or the one taken from NDepend.API) we will tell the tool no to consider that part of the code a dead-code. Ok, so now we now. Let’s get back to our code then.

By introducing changes and splitting our route into two we triggered a new rule to match: Avoid methods with too many local variables. Let’s get rid of it now. The change is quite simple. We will get right of some temp variables used in those lambdas. So instead of this:

we write this:

It’s still readable and we get rid of issues in FacebookModule.cs – nice. Now let’s break some serious stuff down.

MeetupDataprovider - Our new target class

Our new target class

MeetupDataprovider is, in comparison to other classes, a complex one. No wonder that one of our error is: “Avoid methods too big, too complex”

This method does too much. Let’s break it down a little bit. It deals with groups, requests as well as with the results. Let’s split it.

And with such simple change we get rid of issues in this class too. There are no tests for this part of the code so we should write them, but for now let’s just run that piece of code to see if at least the normal case works as before.

By running this tool we see another issue.

Running the background job does produce some strange results

Back to the future?

If you check up closely the times you would notice that the log entry is from 22:12 and the next scheduled check is for the same day, 12 minutes ago. Ok, inconsistencies between UTC and local time. For sure this is an easy fix so let’s do this. Just add .ToLocalTime() to the nextFireTime.Value.

Next issue is this: ‘Don’t use dangerous threading methods’ and the part where’s it’s notifying us about is here:

The construct with endless loop and Sleep is strange I mus say. I guess the reasoning here was to create a endless job runner. I would rewrite this in a way that we wouldn’t have to resort to Sleep.

it would still allow a graceful shutdown if needed and do not block on Thread.Sleep. What do you think about that?

With that change what’s let is only MongoDbProvider<T>, but the recommendations here are a bit tricky (like: Classes that are candidate to be turned into structures) and I would not like to do such changes without making sure we have all tested, so I would skip that for now. One simple change we can do is to make CollectionExists a static method.

So what now? Are we done? Not quite yet, NDepend is smart enough to show us only portion of our debt so that we are not overwhelmed by it. It has a filter set to 30 minutes – what that means is that debt smaller that that would be hidden. Let’s show some of it.

We got some new hits on FacebookPost.cs and FacebookLike.cs. Let’s focus on them. Some easy fixes are for the rule that states that: ‘Avoid non-readonly static fields’. In this scenario it can be easily done.

We also see some examples of ‘Avoid defining multiple types in a source file’ and let’s have a closer look.

here it’s for sue an error and we can split that into two files. We do that with some other classes has the same pattern. And we removed some of the debt with that change too.

The end?

Fighting with technical debt is hard if we do not do it regularly. It grows over time and after a while we no longer care about our code as we did at the beginning. It’s the same here as in the “broken window theory”. If you do not fix those issues right away – you will have more of those before you know it.